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).
This commit is contained in:
@@ -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(
|
||||
|
||||
+3
-2
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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."""
|
||||
|
||||
Reference in New Issue
Block a user