- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 46
refactor: Clean up state management #729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor: Clean up state management #729
Conversation
| QA
 
 
 
 
 After setting a 1hr interval and resetting state: 
 
 
 
 
 
 
 
 
 
 | 
| Well, that was apparently a bunch of code that didn't do anything. | 
| padding: 12px 10px !important; | ||
| } | ||
|  | ||
| /* Make number inputs for intervals shorter */ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these classes exist in the codebase anymore
| transform: translateX(20px); /* Changed to match login page toggle (20px) */ | ||
| } | ||
|  | ||
| /* Stateful Management Styling */ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Classes aren't used in the code anymore.
| // Stateful management reset button | ||
| const resetStatefulBtn = document.getElementById('reset_stateful_btn'); | ||
| if (resetStatefulBtn) { | ||
| resetStatefulBtn.addEventListener('click', () => this.handleStatefulReset()); | ||
| } | ||
|  | ||
| // Stateful management hours input | ||
| const statefulHoursInput = document.getElementById('stateful_management_hours'); | ||
| if (statefulHoursInput) { | ||
| statefulHoursInput.addEventListener('change', () => { | ||
| this.updateStatefulExpirationOnUI(); | ||
| }); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elements don't exist anymore -- dead code.
| if (settings.stateful_management_hours && document.getElementById('stateful_management_hours')) { | ||
| const intervalInput = document.getElementById('stateful_management_hours'); | ||
| const intervalDaysSpan = document.getElementById('stateful_management_days'); | ||
| const expiresDateEl = document.getElementById('stateful_expires_date'); | ||
|  | ||
| // Update the input value | ||
| intervalInput.value = settings.stateful_management_hours; | ||
|  | ||
| // Update the days display | ||
| if (intervalDaysSpan) { | ||
| const days = (settings.stateful_management_hours / 24).toFixed(1); | ||
| intervalDaysSpan.textContent = `${days} days`; | ||
| } | ||
|  | ||
| // Show updating indicator | ||
| if (expiresDateEl) { | ||
| expiresDateEl.textContent = 'Updating...'; | ||
| } | ||
|  | ||
| // Also directly update the stateful expiration on the server and update UI | ||
| this.updateStatefulExpirationOnUI(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Element doesn't exist anymore -- dead code.
| @@ -3477,227 +3427,30 @@ let huntarrUI = { | |||
| // or use the stored configuredApps status if checkAppConnection updates it. | |||
| this.checkAppConnections(); // Re-check all connections after a save might be simplest | |||
| }, | |||
|  | |||
| // Load stateful management info | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the components on the receiving end of this data exist anymore. Code technically alive, but doing work for no reason.
| console.log(`[getUserTimezone] Using timezone: ${timezone}`); | ||
| return timezone; | ||
| }, | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can trigger any of these anymore -- dead code.
| Exclusively supports the v3 API. | ||
| """ | ||
|  | ||
| import time | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three types of changes:
- Removing legacy state management
- Removing unused variables and imports
- Refactor to lazy formatting for logs
| Exclusively supports the v3 API. | ||
| """ | ||
|  | ||
| import time | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three types of changes:
- Removing legacy state management
- Removing unused variables and imports
- Refactor to lazy formatting for logs
| @@ -1,43 +1,39 @@ | |||
| #!/usr/bin/env python3 | |||
|  | |||
| from flask import Blueprint, request, jsonify | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three types of changes:
- Removing legacy state management
- Removing unused variables and imports
- Refactor to lazy formatting for logs
| Handles missing albums or artists based on configuration. | ||
| """ | ||
|  | ||
| import time | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three types of changes:
- Removing legacy state management
- Removing unused variables and imports
- Refactor to lazy formatting for logs
|  | ||
| import time | ||
| import random | ||
| from typing import Dict, Any, Optional, Callable, List, Union, Set # Added List, Union and Set | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three types of changes:
- Removing legacy state management
- Removing unused variables and imports
- Refactor to lazy formatting for logs
| @@ -1,19 +1,17 @@ | |||
| #!/usr/bin/env python3 | |||
|  | |||
| import socket | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two types of changes:
- Removing unused variables and imports
- Refactor to lazy formatting for logs
| # Module exports | ||
| from src.primary.apps.radarr.missing import process_missing_movies | ||
| from src.primary.apps.radarr.upgrade import process_cutoff_upgrades | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick drive-by cleanup on this one.
| @@ -1,19 +1,17 @@ | |||
| #!/usr/bin/env python3 | |||
|  | |||
| import socket | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two types of changes:
- Removing unused variables and imports
- Refactor to lazy formatting for logs
| """ | ||
|  | ||
| import logging | ||
| import json | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick drive-by cleanup in this file.
| def _get_user_timezone(): | ||
| """Get the user's selected timezone from general settings""" | ||
| try: | ||
| from src.primary.utils.timezone_utils import get_user_timezone | ||
| return get_user_timezone() | ||
| except Exception: | ||
| import pytz | ||
| return pytz.UTC | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using get_user_timezone directly
        
          
                src/primary/background.py
              
                Outdated
          
        
      | # --- State Reset Check --- # | ||
| if has_instance_state_expired(app_type, instance_name): | ||
| app_logger.info("State has expired for %s instance '%s'. Resetting state.", app_type, instance_name) | ||
| reset_instance_state_management(app_type, instance_name) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing an intentional per-instance reset now.
| # Check for stateful management expiration first | ||
| # Stateful management check debug removed to reduce log spam | ||
| check_stateful_expiration() # Call the imported function | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more global reset.
| def load_instance_settings(app_name: str, instance_name: str) -> dict[str, Any]: | ||
| """ | ||
| Load settings that apply to a specific instance of an app. | ||
| Args: | ||
| app_name: The app name (sonarr, radarr, etc.) | ||
| instance_name: The specific instance name to load settings for | ||
| Returns: | ||
| dict: Dictionary containing the instance settings | ||
| """ | ||
| app_settings = load_settings(app_name) | ||
|  | ||
| for instance_settings in app_settings.get("instances", []): | ||
| if instance_settings.get("name") == instance_name: | ||
| return instance_settings | ||
|  | ||
| raise ValueError(f"Instance '{instance_name}' not found for app '{app_name}'") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done all over the place, so might as well formalize it.
| return False | ||
| settings_logger.info("Timezone applied to %s (TZ environment variable)", timezone) | ||
|  | ||
| def validate_timezone(timezone_str: str) -> bool: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is implemented in timezone_utils, so using that one instead.
| @@ -1,234 +0,0 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Bye, legacy2 system
| from src.primary.utils.database import get_database | ||
| from src.primary.settings_manager import get_advanced_setting | ||
|  | ||
| def initialize_lock_file() -> None: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Bye, legacy system
|  | ||
| def get_state_management_summary(app_type: str, instance_name: str, instance_hours: int = None) -> Dict[str, Any]: | ||
|  | ||
| def get_instance_state_management_summary(app_type: str, instance_name: str) -> dict[str, Any]: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now figures everything out itself, instead of requiring the caller for part of the summary.
        
          
                src/primary/stateful_manager.py
              
                Outdated
          
        
      |  | ||
|  | ||
| def get_next_reset_time_for_instance(instance_hours: int, app_type: str = None) -> Optional[str]: | ||
| def reset_instance_state_management(app_type: str, instance_name: str) -> bool: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New business logic for resetting instance state here.
| # Create blueprint | ||
| stateful_api = Blueprint('stateful_api', __name__) | ||
|  | ||
| @stateful_api.route('/info', methods=['GET']) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was called, but the info was never used. The client code calling this was removed above.
| response.headers['Access-Control-Allow-Origin'] = '*' | ||
| return response | ||
|  | ||
| def base_response(status: int, data: dict[str, Any]): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done all over the palce in this file. Might as well define what a response should be in one place,
| return response | ||
|  | ||
|  | ||
| @stateful_api.route('/reset', methods=['POST']) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only does per-instance resets now.
| response.headers['Access-Control-Allow-Origin'] = '*' | ||
| return response | ||
|  | ||
| @stateful_api.route('/update-expiration', methods=['POST']) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing called this anymore.
| @@ -1,42 +0,0 @@ | |||
| import socket | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That file wasn't referenced anymore.
| else: | ||
| logger.warning(f"Schedule {schedule_id} not found for update") | ||
|  | ||
| # State Management Methods | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legacy2 system functionality removed.
| global _database_instance | ||
| if _database_instance is None: | ||
| _database_instance = HuntarrDatabase() | ||
| return _database_instance | ||
| if not hasattr(get_database, "db_instance"): | ||
| get_database.db_instance = HuntarrDatabase() | ||
| return get_database.db_instance | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Global was only used in one function, so no point of having a global variable for this. Function attributes should work better here.
| def _get_user_timezone(self): | ||
| """Get the user's selected timezone from general settings""" | ||
| try: | ||
| from src.primary.utils.timezone_utils import get_user_timezone | ||
| return get_user_timezone() | ||
| except Exception: | ||
| # Final fallback if timezone_utils can't be imported | ||
| import pytz | ||
| return pytz.UTC | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using get_user_timezone directly now
| return tz | ||
|  | ||
|  | ||
| def get_timezone_name() -> str: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused.
| """Redirect to main index with user section""" | ||
| return redirect('./#user') | ||
|  | ||
| # This section previously contained code for redirecting paths to include the base URL | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this is dead code that is hidden after the return you see right above this section.
| # Update expiration timing from general settings if applicable | ||
| try: | ||
| new_hours = int(data.get('stateful_management_hours')) | ||
| if new_hours > 0: | ||
| general_logger.info(f"Updating stateful expiration to {new_hours} hours.") | ||
| update_lock_expiration(hours=new_hours) | ||
| except (ValueError, TypeError, KeyError): | ||
| # Don't update if the value wasn't provided or is invalid | ||
| pass | ||
| except Exception as e: | ||
| general_logger.error(f"Error updating expiration timing: {e}") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove legacy system update.
| # Initialize state management system | ||
| try: | ||
| from src.primary.stateful_manager import initialize_state_management | ||
| initialize_state_management() | ||
| except Exception as e: | ||
| huntarr_logger.warning("Failed to initialize state management system: %s", e) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just initialize once on startup, instead of trying to do it on every action
| # Initialize state management for any new instances configured | ||
| initialize_state_management() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also initialize on any updates. This and the startup initialization should cover existing + new tables that could be missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More testing
Fresh install
First boot up
Logs:
INFO:stateful_manager:Initialized state management for sonarr/Default with 168h interval
ERROR:stateful_manager:Skipping initialization of instance 0 in radarr app. Missing setting: 'enabled'
INFO:stateful_manager:Initialized state management for lidarr/Default with 168h interval
INFO:stateful_manager:Initialized state management for readarr/Default with 168h interval
INFO:stateful_manager:Initialized state management for whisparr/Default with 168h interval
INFO:stateful_manager:Initialized state management for eros/Default with 168h interval
DB:
sqlite> select * from stateful_instance_locks;
1|sonarr|Default|1758643604|1759248404|168|2025-09-23 16:06:44
2|lidarr|Default|1758643605|1759248405|168|2025-09-23 16:06:45
3|readarr|Default|1758643605|1759248405|168|2025-09-23 16:06:45
4|whisparr|Default|1758643605|1759248405|168|2025-09-23 16:06:45
5|eros|Default|1758643605|1759248405|168|2025-09-23 16:06:45
Looks good. Note the radarr error is actually an error with the default settings missing the "enabled" key. In general, it's good to report unexpected things, instead of making a best guess an swallowing the error, hence the choice to avoid defaulting to False in this case.
Saving a new instance
Logs:
INFO:stateful_manager:Initialized state management for sonarr/new instance test with 48h interval
DB:
sqlite> select * from stateful_instance_locks;
1|sonarr|Default|1758643604|1759248404|168|2025-09-23 16:06:44
2|lidarr|Default|1758643605|1759248405|168|2025-09-23 16:06:45
3|readarr|Default|1758643605|1759248405|168|2025-09-23 16:06:45
4|whisparr|Default|1758643605|1759248405|168|2025-09-23 16:06:45
5|eros|Default|1758643605|1759248405|168|2025-09-23 16:06:45
6|sonarr|new instance test|1758643766|1758816566|48|2025-09-23 16:09:26
Looks good ✅. New instance is there with correct interval.
Running a cycle
Logs:
INFO - === [SONARR] Thread starting ===
INFO - === Starting SONARR cycle ===
INFO:stateful_manager:State check for sonarr.new instance test: 0.0 hours since last reset (interval: 48h)
INFO:stateful_manager:is_processed check: sonarr/new instance test, ID:58_1, Found:False, Total IDs:0
INFO:stateful_manager:is_processed check: sonarr/new instance test, ID:56_9, Found:False, Total IDs:0
INFO:stateful_manager:is_processed check: sonarr/new instance test, ID:57_1, Found:False, Total IDs:0
INFO - Found 3 unprocessed seasons out of 3 total seasons with cutoff unmet episodes.
INFO:stateful_manager:is_processed check: sonarr/new instance test, ID:58_1, Found:False, Total IDs:0
INFO:stateful_manager:[add_processed_id] Added ID 58_1 to database for sonarr/new instance test
INFO:stateful_manager:is_processed check: sonarr/new instance test, ID:4323, Found:False, Total IDs:1
INFO:stateful_manager:[add_processed_id] Added ID 4323 to database for sonarr/new instance test
INFO:stateful_manager:is_processed check: sonarr/new instance test, ID:4320, Found:False, Total IDs:2
INFO:stateful_manager:[add_processed_id] Added ID 4320 to database for sonarr/new instance test
INFO:stateful_manager:is_processed check: sonarr/new instance test, ID:4318, Found:False, Total IDs:3
INFO:stateful_manager:[add_processed_id] Added ID 4318 to database for sonarr/new instance test
INFO - === SONARR cycle finished. Processed items across instances. ===
INFO - === STATE MANAGEMENT SUMMARY FOR SONARR ===
INFO -   new instance test: 4 items tracked, next reset: 2025-09-25 17:06:39 (48h interval)
INFO - RESULT: Items processed successfully. Total tracked across instances: 4
DB:
sqlite> select * from stateful_processed_ids;
1|sonarr|new instance test|58_1|2025-09-23 17:07:11
2|sonarr|new instance test|4323|2025-09-23 17:07:11
3|sonarr|new instance test|4320|2025-09-23 17:07:11
4|sonarr|new instance test|4318|2025-09-23 17:07:11
Looks good ✅.
| And for a final sanity check: deploying these changes to my original, untouched  I'll be running this version on my system instead and fix anything that comes up. Otherwise, gonna call it here. | 
Description
This started with taking a stab at
I kept discovering code that simultaneously seemed to be causing the problem but also doing nothing. Turns out there's multiple unused legacy state management systems chugging along in the background of the application and one of them is interfering with the current per-instance system.
Instead of trying to prevent the old systems from interfering with the new system, I just got rid of the old systems.
Testing
Built an image using this branch.
Made a clone of my existing huntarr setup.
Ran the modified image instead of the production one.
At this point, it's part of my bigger setup and I'm keeping track of logs about sonarr and radarr cycles (time to reset, tracked items, etc.). Also paying attention to the relevant tables:
Summary of results
Opportunistic fixes & improvements
Fix state management summaries
State management summaries were broken; this whole block does not run because
configured_instancesdoesn't exist. As a result, we always use defaults for a few details in the summary -- the reset interval being the most obvious.Logs below with the error and the default interval, despite a 72h interval configured for sonar:
Now:
Reduce reinitializations and reset checks
Currently state management is mostly handled within
is_processedandadd_processed_id. These are called a lot during each cycle, so you end up doing a settings load and running several queries many times unnecessarily during each cycle. The lifecycle of instances is defined: some exist on startup and the rest are added through settings. If you initialize state management at both these points, you can avoid redoing it many times every cycle and simplify the core loop of the application.Cleanup of timezone logic
Looks like there was a partial centralization of
get_user_timestamplogic to a shared place, but the original duplicated implementations were left as wrappers of the centralized logic. The wrappers were removed because they didn't differ in any way from the function they were wrapping.Separation of concerns
In general, there are layers of the app doing the job of another's. The scope of the changes here are to limited to instance level state management, so only a subset of relevant parts were moved around. More specifically, some business logic was moved out of routes, background cycle coordination, and database layers and into business logic layers.
Dead code removal
There were a few orphaned functions and commented out code from previous refactors. Using the repo history to reference old code is much more sustainable, so that code was removed.
Alignment with PEP8
Python has long-standing, well-defined standards around whitespace, spacing, and imports; these should be followed to reduce friction during development. These were addressed (mostly automatically by an IDE) in the files that were touched.
References:
Lazy formatting in logging
It's generally a good practice to use lazy formatting when logging. While less common of a practice than following PEP8, it is still a common standard that ought to be aligned to unless there's good reason not to. Logging statements were converted (using automation) in the files that were touched.
References: