From e8000b42e72c81ef293d0ad7628e7909fa3a77dc Mon Sep 17 00:00:00 2001 From: Ethan <23321960+0xLeathery@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:23:19 +1000 Subject: [PATCH] fix: prevent stale lock-skip signal leaking between compress_context calls Advisor review found a critical stale-signal leak: if auto-compress sets _compression_skipped_due_to_lock during a lock-skip, a subsequent successful manual /compress will see the stale signal, falsely report 'Compression already in progress', and discard the compression results. Fix: - compress_context clears _compression_skipped_due_to_lock = None at entry so each call's outcome alone determines the signal. - Unified gateway 'holder: unknown' drift to match CLI/TUI pattern (omit holder clause when not a descriptive string). - Added MagicMock opt-outs in 3 sibling test files broken by the new signal check (test_compress_here, test_compress_focus, test_compress_plugin_engine). - Added stale-signal-leak invariant test proving the fix. --- agent/conversation_compression.py | 6 ++ gateway/slash_commands.py | 7 ++- tests/agent/test_compress_signal_leak.py | 60 ++++++++++++++++++++ tests/cli/test_compress_here.py | 1 + tests/gateway/test_compress_focus.py | 2 + tests/gateway/test_compress_plugin_engine.py | 1 + 6 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 tests/agent/test_compress_signal_leak.py diff --git a/agent/conversation_compression.py b/agent/conversation_compression.py index 8360b46e892f..4057ff35a271 100644 --- a/agent/conversation_compression.py +++ b/agent/conversation_compression.py @@ -1283,6 +1283,12 @@ def compress_context( _try_acquire_lock = None _lock_lookup_error: Optional[Exception] = None _legacy_session_db_without_lock_api = False + # Clear any stale lock-skip signal from a prior call so this call's + # outcome alone determines what callers see. Without this an + # auto-compress lock-skip followed by a successful manual /compress + # would falsely report "Compression already in progress" and discard + # the compression results. + agent._compression_skipped_due_to_lock = None if _lock_db is not None: try: _legacy_session_db_without_lock_api = _lock_api_is_absent_on_session_db( diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index d4bd8e2ca4dd..0817b7fafdf3 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -3570,10 +3570,11 @@ class GatewaySlashCommandsMixin: # "No changes from compression" no-op text. _lock_skipped = getattr(tmp_agent, "_compression_skipped_due_to_lock", None) if _lock_skipped: - holder = _lock_skipped if isinstance(_lock_skipped, str) else "unknown" + holder = _lock_skipped if isinstance(_lock_skipped, str) else None + holder_clause = f" (holder: {holder})" if holder else "" return ( - f"⏳ Compression already in progress for this session " - f"(holder: {holder}). Please wait for it to finish." + f"⏳ Compression already in progress for this session" + f"{holder_clause}. Please wait for it to finish." ) if partial and tail: diff --git a/tests/agent/test_compress_signal_leak.py b/tests/agent/test_compress_signal_leak.py new file mode 100644 index 000000000000..aa69af8f90ff --- /dev/null +++ b/tests/agent/test_compress_signal_leak.py @@ -0,0 +1,60 @@ +"""Invariant: stale signal leak between consecutive compress_context calls.""" +from unittest.mock import MagicMock, patch + +import pytest + + +def test_signal_cleared_on_entry_between_calls(monkeypatch): + """Call 1: lock held → signal set. Call 2: lock available → signal must + be None after call 2 because the entry code cleared it. This prevents + a prior auto-compress lock-skip from causing a subsequent successful + manual /compress to falsely report 'Compression already in progress'.""" + from agent.conversation_compression import compress_context + + agent = MagicMock() + agent._cached_system_prompt = "" + agent.tools = None + agent._memory_manager = None + agent._build_system_prompt = MagicMock(return_value="sys prompt") + agent._emit_warning = MagicMock() + + msgs = [ + {"role": "user", "content": "a"}, {"role": "assistant", "content": "b"}, + {"role": "user", "content": "c"}, {"role": "assistant", "content": "d"}, + ] + + monkeypatch.setattr( + "agent.conversation_compression._compression_lock_holder", + lambda a: "pid=test:holder", + ) + + # --- Call 1: lock held --- + db1 = MagicMock() + db1.try_acquire_compression_lock.return_value = False + db1.get_compression_lock_holder.return_value = "pid=holder1" + agent._session_db = db1 + + compress_context(agent, msgs, "", approx_tokens=100, force=True) + + # Call 1: signal set (lock was held). + assert agent._compression_skipped_due_to_lock is not None + + # --- Call 2: lock available, compressor succeeds --- + db2 = MagicMock() + db2.try_acquire_compression_lock.return_value = True + agent._session_db = db2 + agent.context_compressor = MagicMock() + compressed = [ + {"role": "user", "content": "[summary]"}, + {"role": "assistant", "content": "ok"}, + ] + agent.context_compressor.compress = MagicMock(return_value=compressed) + agent.context_compressor.compression_count = 0 + agent.context_compressor.last_compression_rough_tokens = 0 + + compress_context(agent, msgs, "", approx_tokens=100, force=True) + + # Call 2: signal must be None — entry code cleared stale Call 1 signal. + assert agent._compression_skipped_due_to_lock is None, ( + "stale signal from lock-skip call 1 leaked into successful call 2" + ) diff --git a/tests/cli/test_compress_here.py b/tests/cli/test_compress_here.py index 115a12539e5a..bb5bb5c945b2 100644 --- a/tests/cli/test_compress_here.py +++ b/tests/cli/test_compress_here.py @@ -26,6 +26,7 @@ def _wire_agent(shell, compressed_head): shell.agent.session_id = None shell.agent.tools = None shell.agent._compress_context.return_value = (compressed_head, "") + shell.agent._compression_skipped_due_to_lock = False def test_compress_here_compresses_head_only(capsys): diff --git a/tests/gateway/test_compress_focus.py b/tests/gateway/test_compress_focus.py index 100cba800b8b..710bba550315 100644 --- a/tests/gateway/test_compress_focus.py +++ b/tests/gateway/test_compress_focus.py @@ -68,6 +68,7 @@ async def test_compress_focus_topic_passed_to_agent(): agent_instance.context_compressor.has_content_to_compress.return_value = True agent_instance.session_id = "sess-1" agent_instance._compress_context.return_value = (compressed, "") + agent_instance._compression_skipped_due_to_lock = False def _estimate(messages): return 100 @@ -98,6 +99,7 @@ async def test_compress_no_focus_passes_none(): agent_instance.context_compressor.has_content_to_compress.return_value = True agent_instance.session_id = "sess-1" agent_instance._compress_context.return_value = (list(history), "") + agent_instance._compression_skipped_due_to_lock = False with ( patch("gateway.run._resolve_runtime_agent_kwargs", return_value={"api_key": "***"}), diff --git a/tests/gateway/test_compress_plugin_engine.py b/tests/gateway/test_compress_plugin_engine.py index 79eef5551f0a..e20d66cee71f 100644 --- a/tests/gateway/test_compress_plugin_engine.py +++ b/tests/gateway/test_compress_plugin_engine.py @@ -127,6 +127,7 @@ async def test_compress_works_with_plugin_context_engine(): agent_instance.context_compressor = plugin_engine agent_instance.session_id = "sess-1" agent_instance._compress_context.return_value = (compressed, "") + agent_instance._compression_skipped_due_to_lock = False with ( patch("gateway.run._resolve_runtime_agent_kwargs", return_value={"api_key": "***"}),