mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
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.
This commit is contained in:
parent
07cb4a697e
commit
e8000b42e7
6 changed files with 74 additions and 3 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
60
tests/agent/test_compress_signal_leak.py
Normal file
60
tests/agent/test_compress_signal_leak.py
Normal file
|
|
@ -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"
|
||||
)
|
||||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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": "***"}),
|
||||
|
|
|
|||
|
|
@ -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": "***"}),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue