fix:events_in_the_past #149
Conversation
don't allow event scheduler to schedule events until clock has been synced since first boot some systems will think we are still in the last century at boot time! the system.clock.synced event needs to be emitted externally by the OS itself, eg OpenVoiceOS/raspOVOS@3ab5f44#diff-390ab68cb47cd597ab61ab48741f9d35ce7a7d514a4a40014df1222d402bead8R8
WalkthroughThe changes in this pull request focus on enhancing the Changes
Poem
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 (
|
don't allow event scheduler to schedule events until clock has been synced since first boot some systems will think we are still in the last century at boot time! the system.clock.synced event needs to be emitted externally by the OS itself, eg OpenVoiceOS/raspOVOS@3ab5f44#diff-390ab68cb47cd597ab61ab48741f9d35ce7a7d514a4a40014df1222d402bead8R8
![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
🧹 Outside diff range and nitpick comments (3)
ovos_bus_client/util/scheduler.py (3)
78-82: Handle exceptions during file operations for robustness.Currently, file operations assume success, which may lead to unhandled exceptions if an error occurs (e.g., permission issues, disk full). It's good practice to handle potential exceptions to prevent crashes.
Consider wrapping file operations in try-except blocks:
try: with open(self.clock_cache) as f: self.last_sync = float(f.read()) except Exception as e: LOG.error(f"Failed to read clock cache: {e}") self.last_sync = time.time()And when writing:
try: with open(self.clock_cache, "w") as f: f.write(str(self.last_sync)) except Exception as e: LOG.error(f"Failed to write clock cache: {e}")
223-223: Clarify the log message for scheduling events in the past.The log message
"Refusing to schedule event, it is in the past!"could be more informative by specifying the event name or the scheduled time to aid in debugging.Consider enhancing the log message:
-LOG.error("Refusing to schedule event, it is in the past!") +LOG.error(f"Refusing to schedule event '{event}', scheduled time {sched_time} is in the past!")
85-86: Use consistent time formatting throughout the code.Ensure that the time formatting for
pretty_last_syncis consistent in both the__init__method and thehandle_system_clock_syncmethod. This improves readability and consistency in logs.Double-check that the date format strings align in both methods.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
ovos_bus_client/util/scheduler.py (1)
Line range hint
1-391: Add unit tests for clock synchronization logic.The new clock synchronization functionality should be thoroughly tested to ensure reliability.
Would you like me to help create unit tests that cover:
- Clock sync file handling
- Past date detection
- Event scheduling with unsynchronized clock
- Clock sync message handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_bus_client/util/scheduler.py(6 hunks)
🔇 Additional comments (1)
ovos_bus_client/util/scheduler.py (1)
85-85:
Replace hardcoded date with relative time threshold.
Using a hardcoded future date (datetime.datetime(day=1, month=12, year=2024)) will require regular updates and may cause issues after that date passes.
Consider using a relative time difference instead:
-self._past_date = datetime.datetime(day=1, month=12, year=2024)
+self._min_valid_timestamp = time.time() - datetime.timedelta(days=30).total_seconds() # Assume clock shouldn't be more than 30 days in the pastThen update the checks to use this timestamp instead of the datetime comparison.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_bus_client/util/scheduler.py(6 hunks)
🔇 Additional comments (4)
ovos_bus_client/util/scheduler.py (4)
217-224: 🛠️ Refactor suggestion
Improve clock sync validation logic.
The current implementation checks against a fixed date. Consider using a relative time check and add more detailed logging.
Apply this improvement:
-if datetime.datetime.fromtimestamp(self.last_sync) < self._past_date:
+current_time = time.time()
+if current_time - self.last_sync > (180 * 24 * 60 * 60): # 180 days
LOG.error(f"Refusing to schedule event, system clock is not synchronized! "
f"Last sync: {datetime.datetime.fromtimestamp(self.last_sync)}, "
f"Current time: {datetime.datetime.fromtimestamp(current_time)}")
self._dropped_events += 1
returnLikely invalid or redundant comment.
73-82:
Add error handling for file operations.
The file operations for reading and writing the clock sync timestamp should include error handling to handle I/O errors gracefully.
Apply this improvement:
clock_cache = get_xdg_cache_save_path("ovos_clock")
os.makedirs(clock_cache, exist_ok=True)
self.clock_cache = os.path.join(clock_cache, "ovos_clock_sync.ts")
self.last_sync = time.time()
try:
if os.path.isfile(self.clock_cache):
with open(self.clock_cache) as f:
self.last_sync = float(f.read().strip())
else:
with open(self.clock_cache, "w") as f:
f.write(str(self.last_sync))
except (IOError, ValueError) as e:
LOG.error(f"Error handling clock sync file: {e}")
# Keep default self.last_sync valueLikely invalid or redundant comment.
278-288:
Improve robustness of clock sync handler.
The handler needs input validation and atomic file operations to prevent corruption.
Apply these improvements:
def handle_system_clock_sync(self, message: Message):
+ if not message or not isinstance(message, Message):
+ LOG.error("Invalid message received in clock sync handler")
+ return
+
# clock sync, are we in the past?
- if datetime.datetime.fromtimestamp(self.last_sync) < self._past_date:
+ current_time = time.time()
+ if current_time - self.last_sync > (180 * 24 * 60 * 60): # 180 days
LOG.warning(f"Clock was in the past!!! {self._dropped_events} scheduled events have been dropped")
- self.last_sync = time.time()
+ self.last_sync = current_time
pretty_last_sync = datetime.datetime.fromtimestamp(self.last_sync).strftime("%Y-%m-%d %H:%M:%S")
LOG.info(f"clock sync: {pretty_last_sync}")
- with open(self.clock_cache, "w") as f:
- f.write(str(self.last_sync))
+ # Write to temporary file first to ensure atomic update
+ temp_file = f"{self.clock_cache}.tmp"
+ try:
+ with open(temp_file, "w") as f:
+ f.write(str(self.last_sync))
+ os.replace(temp_file, self.clock_cache)
+ except IOError as e:
+ LOG.error(f"Failed to update clock sync file: {e}")Likely invalid or redundant comment.
85-85:
Replace hardcoded date with a configurable threshold.
Using a hardcoded date (datetime.datetime(day=1, month=12, year=2024)) for clock validation is not future-proof. Consider using a configurable time delta instead.
Apply this improvement:
-self._past_date = datetime.datetime(day=1, month=12, year=2024)
+# Default to 6 months before current time
+self._min_timestamp = time.time() - (180 * 24 * 60 * 60) # 180 days in secondsLikely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #149 +/- ##
==========================================
+ Coverage 39.49% 42.33% +2.84%
==========================================
Files 17 16 -1
Lines 1732 2178 +446
==========================================
+ Hits 684 922 +238
- Misses 1048 1256 +208 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ovos_bus_client/util/scheduler.py (2)
21-23: Remove unused import.The
osmodule is imported but not used directly. It's likely indirectly used throughos.path, which is already imported.-import os🧰 Tools
🪛 Ruff (0.8.0)
23-23:
osimported but unusedRemove unused import:
os(F401)
208-216: Improve clock synchronization check robustness.The current implementation could be more robust by considering edge cases and providing more detailed logging.
-if datetime.datetime.fromtimestamp(self._last_sync) < self._past_date: +current_time = time.time() +if current_time - self._last_sync > datetime.timedelta(hours=6).total_seconds(): # this works around problems in raspOVOS images and other # systems without RTC that didnt sync clock with the internet yet LOG.error("Refusing to schedule event, system clock is in the past!") + LOG.debug(f"Last sync: {datetime.datetime.fromtimestamp(self._last_sync)}, " + f"Current time: {datetime.datetime.fromtimestamp(current_time)}") self._dropped_events += 1 return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_bus_client/util/scheduler.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
ovos_bus_client/util/scheduler.py
23-23: os imported but unused
Remove unused import: os
(F401)
🔇 Additional comments (5)
ovos_bus_client/util/scheduler.py (5)
99-99: Document system.clock.synced message bus event.
The system.clock.synced event is crucial for preventing scheduling issues when the system clock is in the past. This event and its purpose should be documented.
254-254: Correct the error message to reflect the missing parameter.
The error message mentions 'msg_type' but the expected parameter is 'event'.
78-80: LGTM! Good logging improvements.
The addition of human-readable timestamps in the logs will help with debugging clock-related issues.
74-77:
Replace hardcoded date with a dynamic approach.
Using a hardcoded date (datetime.datetime(day=1, month=12, year=2024)) for clock synchronization checks is not future-proof. Consider using a relative time comparison instead.
-self._past_date = datetime.datetime(day=1, month=12, year=2024)
+# Use a timestamp from 24 hours ago as a reasonable threshold
+self._past_date = datetime.datetime.now() - datetime.timedelta(days=1)Likely invalid or redundant comment.
269-278: 🛠️ Refactor suggestion
Improve error handling in clock sync handler.
The clock synchronization handler should validate the message and handle potential errors.
def handle_system_clock_sync(self, message: Message):
+ if not message or not isinstance(message, Message):
+ LOG.error("Invalid message received in clock sync handler")
+ return
+
# clock sync, are we in the past?
if datetime.datetime.fromtimestamp(self._last_sync) < self._past_date:
LOG.warning(f"Clock was in the past!!! {self._dropped_events} scheduled events have been dropped")
self._last_sync = time.time()
- # Convert Unix timestamp to human-readable datetime
pretty_last_sync = datetime.datetime.fromtimestamp(self._last_sync).strftime("%Y-%m-%d %H:%M:%S")
LOG.info(f"clock sync: {pretty_last_sync}")Likely invalid or redundant comment.
don't allow event scheduler to schedule events until clock has been synced since first boot
some systems will think we are still in the last century at boot time!
the system.clock.synced event needs to be emitted externally by the OS itself, eg OpenVoiceOS/raspOVOS@3ab5f44#diff-390ab68cb47cd597ab61ab48741f9d35ce7a7d514a4a40014df1222d402bead8R8
Summary by CodeRabbit
New Features
Bug Fixes
Documentation