diff --git a/emrg/session.py b/emrg/session.py index 28fc9cff..52dbf5a5 100644 --- a/emrg/session.py +++ b/emrg/session.py @@ -913,6 +913,31 @@ def append_llm(self, record: dict) -> None: with open(self._llm_path, "a", encoding="utf-8") as f: f.write(line) + def _stray_generations(self) -> list[Path]: + """Backup generations numbered at or above ``_LLM_LOG_BACKUP_COUNT``. + + The directory is listed and each name read, rather than the numbering being + counted up to the first absent one: a gap must not hide the generations past + it. Only a name that is exactly ``.`` is a generation — the + suffix has to be ASCII digits, so no other file and no spelled-out numeral is + taken for one. + """ + prefix = self._llm_path.name + "." + try: + entries = list(self._llm_path.parent.iterdir()) + except OSError: + return [] + strays = [] + for entry in entries: + if not entry.name.startswith(prefix): + continue + suffix = entry.name[len(prefix):] + if not (suffix.isascii() and suffix.isdigit()): + continue + if int(suffix) >= _LLM_LOG_BACKUP_COUNT: + strays.append(entry) + return strays + def _rotate_llm_log(self) -> None: """Rotate llm.jsonl if it exceeds _LLM_LOG_MAX_BYTES.""" if not self._llm_path.exists(): @@ -925,25 +950,19 @@ def _rotate_llm_log(self) -> None: return # Shift the backups down, dropping the oldest: .{n-1} → .n, …, .1 → .2, - # main → .1. The oldest generation is removed outright, so the file set - # is the live log plus _LLM_LOG_BACKUP_COUNT backups and no more. + # main → .1. Every generation numbered at or above the count goes, wherever + # it sits in the numbering, so the set that remains is the live log plus + # _LLM_LOG_BACKUP_COUNT backups and no more. # - # Every generation numbered at or above the count goes, not only the one - # the count names. A session written by the loop this replaces already - # carries a third generation, and leaving it would keep three backups - # for the rest of that session's life — while the shift below would then - # delete the second-newest and preserve the oldest, which is the wrong - # two to keep. - generation = _LLM_LOG_BACKUP_COUNT + 1 - while True: - stray = Path(str(self._llm_path) + f".{generation}") - if not stray.exists(): - break - stray.unlink() - generation += 1 - oldest = Path(str(self._llm_path) + f".{_LLM_LOG_BACKUP_COUNT}") - if oldest.exists(): - oldest.unlink() + # The strays are enumerated, not counted up to. A session can hold .4 with + # no .3 — a numbering left by a directory that was restored, copied or + # edited by hand — and a sweep that stopped at the first absent name would + # leave that generation behind for the rest of the session's life, while + # the shift below deleted the second-newest and preserved the oldest: the + # wrong two to keep. A session written by the loop this replaces carries a + # third generation too, which the same list removes. + for stray in self._stray_generations(): + stray.unlink(missing_ok=True) for i in range(_LLM_LOG_BACKUP_COUNT - 1, 0, -1): old = Path(str(self._llm_path) + f".{i}") if old.exists(): diff --git a/tests/test_session.py b/tests/test_session.py index 9f424c47..0a4f78be 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -1509,3 +1509,58 @@ def test_a_fifo_input_history_does_not_wedge_the_write(self, tmp_path): def _session_dir(self, tmp_path, name="s1"): return tmp_path / ".emrg" / "sessions" / name + + +# Which generations a session already holds, and which two must remain afterwards. +# The numbering is the one the code writes — `llm.jsonl.1` is the newest backup — +# and the last two rows are the states a count-up sweep read wrongly. +ROTATION_STARTING_STATES = [ + ((), {1: "live"}), + ((1,), {1: "live", 2: "old-1"}), + ((1, 2), {1: "live", 2: "old-1"}), + ((1, 2, 3), {1: "live", 2: "old-1"}), + ((1, 2, 3, 4, 5), {1: "live", 2: "old-1"}), + ((1, 2, 4), {1: "live", 2: "old-1"}), + ((1, 3), {1: "live", 2: "old-1"}), +] + + +@pytest.mark.parametrize("backups, expected", ROTATION_STARTING_STATES) +def test_the_rotation_keeps_the_newest_backups_whatever_the_starting_state( + tmp_path, monkeypatch, backups, expected +): + """The survivors are named, because a count cannot tell "wrong two" from "one too many". + + Each backup's first line names the generation that wrote it, so this asserts which + generations remain rather than how many files there are — the two readings that come + apart when a shift keeps the oldest instead of the newest. + + The row carrying a gap past the count (`live .1 .2 .4`, with no `.3`) is the one a + sweep that stopped at the first absent name read wrongly: `.4` survived and the set + never came back to the two generations the count declares, so that row fails against + such a sweep while the other six pass. A gap *below* the count is not the same case — + `.2` is overwritten by the shift whatever was there — and `live .1 .3` is carried to + say so, since it passes either way. + """ + monkeypatch.setattr("emrg.session._LLM_LOG_MAX_BYTES", 64) + + session = Session("s1", tmp_path) + live = session._llm_path + live.write_text("live\n" + "x" * 80, encoding="utf-8") + for generation in backups: + Path(str(live) + f".{generation}").write_text( + f"old-{generation}\n" + "x" * 80, encoding="utf-8" + ) + + session._rotate_llm_log() + + survivors = {} + for entry in sorted(live.parent.iterdir()): + if not entry.name.startswith(live.name + "."): + continue + survivors[entry.name] = entry.read_text(encoding="utf-8").splitlines()[0] + + assert survivors == { + f"{live.name}.{number}": generation + for number, generation in expected.items() + }