From e4d4afd8600d9541d9ac8589b00da6cd0b670c31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20P=C3=A9rez?= Date: Tue, 20 Feb 2024 14:52:25 -0500 Subject: [PATCH] Stop keeping old copies of eventbus files (#941) We've never restored from these "backups", and as long as we ensure that the current file points to a fully written file, it's fine to only keep at most 2 files around: the actual current file and a temporary "new" file. To ensure that we're not pointing at a half-written file, we continue using the age-old pattern for atomic file updates: write to another file and swap a symlink once the write is complete --- tests/eventbus_test.py | 5 ++++- tron/eventbus.py | 23 +++++++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/tests/eventbus_test.py b/tests/eventbus_test.py index da3abf336..ae4606ab5 100644 --- a/tests/eventbus_test.py +++ b/tests/eventbus_test.py @@ -100,7 +100,10 @@ def test_sync_save_log_time(self, time): self.eventbus.sync_save_log("test") new_link = os.readlink(self.eventbus.log_current) assert_equal(new_link, os.path.join(self.log_dir.name, "2.pickle")) - assert os.path.exists(current_link) + # we clean up the previous link so as not to have a million pickles + # on disk + assert not os.path.exists(current_link) + # so at this point, we should only have the new link assert os.path.exists(new_link) @mock.patch("tron.eventbus.time", autospec=True) diff --git a/tron/eventbus.py b/tron/eventbus.py index 8aea37092..de9ad28a3 100644 --- a/tron/eventbus.py +++ b/tron/eventbus.py @@ -5,6 +5,7 @@ import time from collections import defaultdict from collections import deque +from typing import Optional from twisted.internet import reactor @@ -144,9 +145,18 @@ def sync_load_log(self): duration = time.time() - started log.info(f"log read from disk, took {duration:.4}s") - def sync_save_log(self, reason): + def sync_save_log(self, reason: str) -> bool: started = time.time() new_file = os.path.join(self.log_dir, f"{int(started)}.pickle") + previous_file: Optional[str] = os.path.realpath(os.path.join(self.log_dir, "current")) + # if we're starting a fresh Tron server, there won't be a current symlink + # and the above line will give us the path to what will eventually be the current + # symlink...which is undesirable since we clean up whatever this points to :p + # we can tell if this is happening since this previous_file variable should + # always point to a file that ends in .pickle under normal operation + if previous_file and previous_file.endswith("current"): + previous_file = None + try: with open(new_file, "xb") as f: pickle.dump(self.event_log, f) @@ -165,7 +175,16 @@ def sync_save_log(self, reason): except FileNotFoundError: pass os.symlink(new_file, tmplink) - os.replace(tmplink, self.log_current) + os.replace(src=tmplink, dst=self.log_current) + # once we get here, `self.log_current` is now pointing to `new_file` + # so we can safely delete the previous `self.log_current` target without + # fear of losing data + if previous_file: + try: + os.remove(previous_file) + except Exception: + # this shouldn't happen - but we also shouldn't crash if the impossible happens + log.exception(f"unable to delete {previous_file} - continuing anyway.") duration = time.time() - started log.info(f"log dumped to disk because {reason}, took {duration:.4}s")