From 3aee290899e478c5fdfb6a241ef62758a49829b3 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 31 Aug 2026 12:01:29 +0530 Subject: [PATCH] refactor(compression): name the split-failure cooldown; drop duplicate tests Review folds from the formal gate battery: - _SPLIT_FAILURE_COOLDOWN_SECONDS = 60 replaces the bare literal, with a comment pinning WHY it is the timeout ladder's first rung (transient lease/DB condition) rather than the 600s summary-provider cooldown. - publish_compression_child docstring now states the compression_lock_holder condition on the refresh guard. - Dropped 2 of 3 extracted unit tests as duplicates of existing coverage in test_compression_rotation_state.py / test_context_compressor.py; kept the force-bypass test (only site pinning that behavior for split failures) and the E2E test (now asserting the named constant). --- agent/conversation_compression.py | 10 +++++- hermes_state.py | 5 +-- .../agent/test_compression_concurrent_fork.py | 4 ++- ...test_compression_split_failure_cooldown.py | 36 +++---------------- 4 files changed, 20 insertions(+), 35 deletions(-) diff --git a/agent/conversation_compression.py b/agent/conversation_compression.py index 60c62e4bd0..b6f6549870 100644 --- a/agent/conversation_compression.py +++ b/agent/conversation_compression.py @@ -92,6 +92,13 @@ _TERMINAL_COMPRESSION_PROVENANCES = frozenset( } ) +# Cooldown armed when a compression SPLIT fails (session_split_failed / +# rotation rollback, #97948 symptom B). Deliberately the FIRST rung of the +# timeout ladder (60/300/900 in context_compressor.py), not the 600s +# _SUMMARY_FAILURE_COOLDOWN_SECONDS: a split failure is usually a transient +# lease/DB condition, unlike a persistent summary-provider fault. +_SPLIT_FAILURE_COOLDOWN_SECONDS = 60 + # Stable marker the gateway matches on to re-tag the auto-compaction lifecycle # status as ``kind="compacting"`` (tui_gateway/server.py::_status_update), so # drivers like the desktop app can show an explicit "Summarizing…" indicator @@ -5001,7 +5008,8 @@ def compress_context( # a stub compressor must not mask the original error. try: agent.context_compressor._record_compression_failure_cooldown( - 60, f"session_split_failed: {e}", + _SPLIT_FAILURE_COOLDOWN_SECONDS, + f"session_split_failed: {e}", ) except Exception: logger.debug( diff --git a/hermes_state.py b/hermes_state.py index b4e4db3d0c..4b3c8d2742 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -7072,8 +7072,9 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin) rows in ``(watermark, watermark_ceiling]`` are foreign concurrent tail. ``None`` = unbounded (no internal flush happened). - When *require_lease_refresh* is True, the lease is refreshed inside - the same transaction before the expiry check. This gives a refresher + When *require_lease_refresh* is True and *compression_lock_holder* is + set, the lease is refreshed inside the same transaction before the + expiry check. This gives a refresher that stopped due to transient DB failures one final chance to extend the lease, preventing wasted compression work. The refresh uses the same ``conn`` as the publication, so there is no TOCTOU window. diff --git a/tests/agent/test_compression_concurrent_fork.py b/tests/agent/test_compression_concurrent_fork.py index f5d6b9e104..a155b5ab65 100644 --- a/tests/agent/test_compression_concurrent_fork.py +++ b/tests/agent/test_compression_concurrent_fork.py @@ -2233,6 +2233,8 @@ def test_failed_split_arms_failure_cooldown(tmp_path: Path) -> None: """Regression #97948 symptom B: a failed split/archive must arm the compression failure cooldown so the next automatic turn cannot immediately re-run the identical doomed compression.""" + from agent.conversation_compression import _SPLIT_FAILURE_COOLDOWN_SECONDS + db = SessionDB(db_path=tmp_path / "state.db") session_id = "SPLIT_FAIL_COOLDOWN_TEST" db.create_session(session_id, source="test") @@ -2255,5 +2257,5 @@ def test_failed_split_arms_failure_cooldown(tmp_path: Path) -> None: "split failure must arm the failure cooldown (#97948 symptom B)" ) seconds, error = cooldown_calls[0].args - assert seconds == 60 + assert seconds == _SPLIT_FAILURE_COOLDOWN_SECONDS assert "session_split_failed" in str(error) diff --git a/tests/agent/test_compression_split_failure_cooldown.py b/tests/agent/test_compression_split_failure_cooldown.py index 1fb200a8a1..b7a738ec23 100644 --- a/tests/agent/test_compression_split_failure_cooldown.py +++ b/tests/agent/test_compression_split_failure_cooldown.py @@ -1,8 +1,10 @@ """Regression tests: a failed compression split must arm the failure cooldown. -Extracted verbatim from PR #98137's test file (author: vsd2807); the sibling -timeout-reconciliation tests were not carried because that production path was -not salvaged (blocking review on #98137). +Extracted from PR #98137's test file (author: vsd2807); two sibling unit +tests were dropped as duplicates of existing coverage in +test_compression_rotation_state.py / test_context_compressor.py, and the +timeout-reconciliation tests were not carried because that production path +was not salvaged (blocking review on #98137). Issue #97948 symptom B: without the cooldown, the turn after a session_split_failed abort immediately re-runs the identical doomed @@ -12,34 +14,6 @@ compression. import time from unittest.mock import MagicMock -def test_split_failure_records_cooldown(): - """After a session split failure, compression failure cooldown is recorded.""" - from agent.context_compressor import ContextCompressor - - compressor = ContextCompressor.__new__(ContextCompressor) - compressor._summary_failure_cooldown_until = 0.0 - compressor._last_summary_error = None - compressor._cooldown_persist_failed = False - compressor._session_db = None - compressor._session_id = "" - - compressor._record_compression_failure_cooldown(60, "session_split_failed: test") - - assert compressor._summary_failure_cooldown_until > time.monotonic() - assert compressor._last_summary_error == "session_split_failed: test" - - -def test_cooldown_blocks_automatic_compression(): - from agent.context_compressor import ContextCompressor - - compressor = ContextCompressor.__new__(ContextCompressor) - compressor._summary_failure_cooldown_until = time.monotonic() + 60.0 - compressor._last_summary_error = "session_split_failed" - compressor._cooldown_persist_failed = False - compressor.quiet_mode = True - - assert compressor._automatic_compression_blocked_locally() is True - def test_manual_compress_bypasses_cooldown(): """Manual /compress (force=True) bypasses cooldown — existing behavior preserved."""