feat: implement dynamic date filtering for image search #1166#1256
feat: implement dynamic date filtering for image search #1166#1256Varun-JP wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
WalkthroughIntroduced a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sync-microservice/app/utils/watcher.py (1)
336-347:⚠️ Potential issue | 🔴 CriticalCritical: Thread cannot join itself when
restartis called from the worker thread.When a watched folder is deleted,
watcher_util_handle_file_changes(line 89) calls this function from within the worker thread. This causeswatcher_thread.join()to attempt joining the current thread, raisingRuntimeError: cannot join current thread. The exception is caught, state is cleared, and a new thread spawns—while the old worker is still running. This defeats the PR's purpose of preventing multiple watcher threads.🐛 Proposed fix: detect self-join and handle restart differently
def watcher_util_restart_folder_watcher() -> bool: """ Restart the folder watcher by stopping the current one and starting fresh. Returns: True if restart was successful, False otherwise """ + global watcher_thread + with state_lock: + # Check if we're being called from the worker thread itself + if watcher_thread is not None and watcher_thread is threading.current_thread(): + logger.info("Restart requested from worker thread - scheduling async restart") + # Signal stop; a separate mechanism must handle the actual restart + stop_event.set() + return False + logger.info("Restarting folder watcher...") watcher_util_stop_folder_watcher() return watcher_util_start_folder_watcher()Alternatively, consider refactoring
watcher_util_handle_file_changesto signal a restart request rather than performing it synchronously. A separate thread or event loop could then handle the restart after the worker exits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync-microservice/app/utils/watcher.py` around lines 336 - 347, The restart function can try to join the watcher from within the watcher thread causing RuntimeError; update watcher_util_restart_folder_watcher to detect if threading.current_thread() is the active watcher_thread (the watcher_thread variable used by watcher_util_start_folder_watcher / watcher_util_stop_folder_watcher) and avoid calling watcher_thread.join() in that case — instead set a restart_requested flag (or schedule a restart) and return True so the worker can exit cleanly; also update watcher_util_stop_folder_watcher to respect the restart flag and not clear state prematurely, and ensure watcher_util_handle_file_changes signals the restart_requested flag rather than doing a synchronous restart when running inside the worker thread.
🧹 Nitpick comments (3)
sync-microservice/app/utils/watcher.py (3)
32-51: Shared statewatched_foldersis read without lock protection.While list iteration during concurrent assignment is generally safe under CPython's GIL, this relies on implementation details. For consistency with the PR's thread-safety goals, consider protecting reads of
watched_foldershere and inwatcher_util_handle_file_changes(line 75).♻️ Proposed fix for watcher_util_get_folder_id_if_watched
def watcher_util_get_folder_id_if_watched(file_path: str) -> Optional[str]: ... # Normalize the file path normalized_path = os.path.abspath(file_path) + with state_lock: + folders_snapshot = list(watched_folders) + # Check if this path matches any of our watched folders - for folder_id, folder_path in watched_folders: + for folder_id, folder_path in folders_snapshot: if os.path.abspath(folder_path) == normalized_path: return folder_id return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync-microservice/app/utils/watcher.py` around lines 32 - 51, The function watcher_util_get_folder_id_if_watched reads the shared list watched_folders without synchronization; wrap the read/iteration in the same thread-safety lock used elsewhere (e.g., watched_folders_lock) to prevent races, i.e., acquire the lock before normalizing/iterating watched_folders and release it after returning (or use a context manager) and apply the same locking discipline in watcher_util_handle_file_changes to protect all accesses to watched_folders.
349-362: Lock ensures consistent snapshot of watcher state.The implementation correctly reads all state under the lock for consistency.
Consider adding a more specific return type hint for better IDE support:
-def watcher_util_get_watcher_info() -> dict: +def watcher_util_get_watcher_info() -> Dict[str, Any]:You'd also need to add
Anyto the imports fromtyping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync-microservice/app/utils/watcher.py` around lines 349 - 362, The return type of watcher_util_get_watcher_info is currently untyped (dict); change it to a more specific type such as dict[str, Any] (or Mapping[str, Any]) to improve IDE/type support and add Any to the typing imports; update the function signature watcher_util_get_watcher_info() -> dict[str, Any] and ensure the module imports Any from typing alongside any existing typing imports.
364-376: Consider adding lock protection for consistency.This function reads
watcher_threadwithout acquiringstate_lock, which is inconsistent with the thread-safety pattern applied elsewhere. While primarily used in the examplemain(), protecting it ensures consistent behavior if called from other contexts.♻️ Proposed fix
def watcher_util_wait_for_watcher() -> None: """ Wait for the watcher to finish (useful for keeping the program running). """ - if watcher_thread and watcher_thread.is_alive(): + with state_lock: + thread = watcher_thread + + if thread and thread.is_alive(): try: - watcher_thread.join() # Wait indefinitely + thread.join() # Wait indefinitely except KeyboardInterrupt: logger.info("Interrupted by user") watcher_util_stop_folder_watcher() else: logger.info("No watcher thread to wait for")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sync-microservice/app/utils/watcher.py` around lines 364 - 376, Protect reads of the shared watcher_thread with the existing state_lock in watcher_util_wait_for_watcher: acquire state_lock before checking watcher_thread and watcher_thread.is_alive(), store a local reference, release the lock, then perform the blocking join and KeyboardInterrupt handling as before (call watcher_util_stop_folder_watcher() and use logger). This mirrors the thread-safety pattern used elsewhere and keeps use of watcher_thread consistent with state_lock while preserving behavior of watcher_util_wait_for_watcher.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sync-microservice/app/utils/watcher.py`:
- Around line 336-347: The restart function can try to join the watcher from
within the watcher thread causing RuntimeError; update
watcher_util_restart_folder_watcher to detect if threading.current_thread() is
the active watcher_thread (the watcher_thread variable used by
watcher_util_start_folder_watcher / watcher_util_stop_folder_watcher) and avoid
calling watcher_thread.join() in that case — instead set a restart_requested
flag (or schedule a restart) and return True so the worker can exit cleanly;
also update watcher_util_stop_folder_watcher to respect the restart flag and not
clear state prematurely, and ensure watcher_util_handle_file_changes signals the
restart_requested flag rather than doing a synchronous restart when running
inside the worker thread.
---
Nitpick comments:
In `@sync-microservice/app/utils/watcher.py`:
- Around line 32-51: The function watcher_util_get_folder_id_if_watched reads
the shared list watched_folders without synchronization; wrap the read/iteration
in the same thread-safety lock used elsewhere (e.g., watched_folders_lock) to
prevent races, i.e., acquire the lock before normalizing/iterating
watched_folders and release it after returning (or use a context manager) and
apply the same locking discipline in watcher_util_handle_file_changes to protect
all accesses to watched_folders.
- Around line 349-362: The return type of watcher_util_get_watcher_info is
currently untyped (dict); change it to a more specific type such as dict[str,
Any] (or Mapping[str, Any]) to improve IDE/type support and add Any to the
typing imports; update the function signature watcher_util_get_watcher_info() ->
dict[str, Any] and ensure the module imports Any from typing alongside any
existing typing imports.
- Around line 364-376: Protect reads of the shared watcher_thread with the
existing state_lock in watcher_util_wait_for_watcher: acquire state_lock before
checking watcher_thread and watcher_thread.is_alive(), store a local reference,
release the lock, then perform the blocking join and KeyboardInterrupt handling
as before (call watcher_util_stop_folder_watcher() and use logger). This mirrors
the thread-safety pattern used elsewhere and keeps use of watcher_thread
consistent with state_lock while preserving behavior of
watcher_util_wait_for_watcher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26da2d06-4c21-42f0-b597-3c50755db897
📒 Files selected for processing (1)
sync-microservice/app/utils/watcher.py
|
PR without issue assignment is not appreciated please wait for reviewers to workout and assign your issues first :) |
|
Hi @SIDDHANTCOOKIE, Thanks for the clarification, and apologies for opening the PR without prior assignment. |
2 participants
Addressed Issues:
Fixes #1166
Screenshots/Recordings:
N/A (backend/thread-safety fix, no UI changes)
Additional Notes:
threading.RLock()insync-microservice/app/utils/watcher.pyto protect shared watcher state.watcher_util_start_folder_watcherwatcher_util_stop_folder_watcherwatcher_util_restart_folder_watcherwatcher_util_is_watcher_runningwatcher_util_get_watcher_info/startrequests from spawning multiple watcher threads (TOCTOU race condition).AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools:
Checklist
Summary by CodeRabbit