fix(compress): tell the user when /compress no-ops because another compression holds the lock - #69870
Merged
Conversation
Contributor
૮ >ﻌ< ა ci reviewran on 1cca01c ℹ️ InfoDesktop E2E visual evidence · View test artifacts · View job1 visual diff. inline evidence upload failed. Failed to upload diff-665a0833239e-onboarding-overlay-diff.png with gh image (exit code 1): Error uploading /home/runner/work/_temp/e2e-evidence/diff-665a0833239e-onboarding-overlay-diff.png: step 0 (get upload token): uploadToken not found on repo page — do you have write access to NousResearch/hermes-agent? (or, if NousResearch enforces SAML SSO, authorize at https://github.com/orgs/NousResearch/sso) |
alt-glitch
added
type/bug
…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.
…ll manual-compress surfaces Follow-up to the salvaged #57634 commits: - agent/manual_compression_feedback.py: new describe_compression_lock_skip() — single source of truth for lock-skip wording. A descriptive holder string means another compressor CONFIRMED holds the lock ('already in progress (holder: ...)'); True/None means acquisition failed without a confirmed holder (hermes_state.try_acquire_compression_lock catches sqlite3.Error internally and returns False), so the message says 'could not acquire ... the lock check failed' instead of falsely claiming a concurrent compression is running. - cli.py, gateway/slash_commands.py, tui_gateway/server.py (all three in-process consumers: session.compress RPC, command.dispatch compress branch, slash.exec mirror) now route through the shared helper. - tui_gateway/server.py command.dispatch compress branch: catch CompressionLockHeld explicitly — it previously fell into the generic 'compress failed' error handler. - Deferred-notify contract (#69324): lock-skip discards the pending context-engine notification (committed=False) in _compress_session_history and the CLI path before returning. - tests: lock-skip wording pins per surface, VISIBLE_COMPRESSION_MESSAGES noise-filter carve-outs for both wordings, MagicMock signal opt-outs for sibling tests added on main after the original PR.
teknium1
force-pushed
the
salvage/57634-lock-reason
branch
from
July 23, 2026 14:36
33cb10a to
5119144
Compare
The bare truthiness test on _compression_skipped_due_to_lock is fooled by MagicMock auto-attributes on test-double agents (skill pitfall: MagicMock defeats hasattr/truthiness duck-typing) — the type-ahead CLI test's MagicMock agent took the lock-skip branch and skipped the transcript commit. Real values are None/True/holder-string; pin the check to 'is True or isinstance(str)' at all three consumer sites.
This was referenced Jul 23, 2026
teknium1
pushed a commit
that referenced
this pull request
Jul 23, 2026
… exhausting A lock-loser compression pass returns its input unchanged, which the automatic compression sites misread as 'cannot compress further': the preflight loop armed the insufficient-progress blocker, the pre-API gate burned a shared attempt, and a lock-contended 413/overflow retried into the attempt cap and returned compression_exhausted — which the gateway answers with a full session auto-reset (#9893/#35809). A temporary concurrent-compression defer wiped the session. Consume the landed #69870 lock-skip signal on every automatic path (preflight in turn_context, pre-API pressure gate, 413 handler, overflow handler, post-tool compaction): when a pass no-ops AND the type-pinned lock-skip flag is set, refund the attempt (never count it toward the cap or the insufficient-progress blocker), and when the turn cannot proceed (provider already proved the request does not fit) end it with a soft compression_deferred result — distinct from compression_exhausted — so the gateway keeps the session intact and the next message retries after the concurrent compressor finishes. The new compression_skipped_due_to_lock() reader is type-pinned (is True or isinstance(str)) per the MagicMock auto-attribute rule, and compress_context() now also clears the signal at the very top of every attempt (per-attempt state rule, #58629/#69853) so a stale value can never make a later breaker/codex no-op look like lock contention. Salvaged from PR #49874; rebuilt on main's #69870 _compression_skipped_due_to_lock signal instead of the PR's parallel _compression_deferred_by_lock triple.
teknium1
pushed a commit
that referenced
this pull request
Jul 23, 2026
… exhausting A lock-loser compression pass returns its input unchanged, which the automatic compression sites misread as 'cannot compress further': the preflight loop armed the insufficient-progress blocker, the pre-API gate burned a shared attempt, and a lock-contended 413/overflow retried into the attempt cap and returned compression_exhausted — which the gateway answers with a full session auto-reset (#9893/#35809). A temporary concurrent-compression defer wiped the session. Consume the landed #69870 lock-skip signal on every automatic path (preflight in turn_context, pre-API pressure gate, 413 handler, overflow handler, post-tool compaction): when a pass no-ops AND the type-pinned lock-skip flag is set, refund the attempt (never count it toward the cap or the insufficient-progress blocker), and when the turn cannot proceed (provider already proved the request does not fit) end it with a soft compression_deferred result — distinct from compression_exhausted — so the gateway keeps the session intact and the next message retries after the concurrent compressor finishes. The new compression_skipped_due_to_lock() reader is type-pinned (is True or isinstance(str)) per the MagicMock auto-attribute rule, and compress_context() now also clears the signal at the very top of every attempt (per-attempt state rule, #58629/#69853) so a stale value can never make a later breaker/codex no-op look like lock contention. Salvaged from PR #49874; rebuilt on main's #69870 _compression_skipped_due_to_lock signal instead of the PR's parallel _compression_deferred_by_lock triple.
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
… exhausting A lock-loser compression pass returns its input unchanged, which the automatic compression sites misread as 'cannot compress further': the preflight loop armed the insufficient-progress blocker, the pre-API gate burned a shared attempt, and a lock-contended 413/overflow retried into the attempt cap and returned compression_exhausted — which the gateway answers with a full session auto-reset (NousResearch#9893/NousResearch#35809). A temporary concurrent-compression defer wiped the session. Consume the landed NousResearch#69870 lock-skip signal on every automatic path (preflight in turn_context, pre-API pressure gate, 413 handler, overflow handler, post-tool compaction): when a pass no-ops AND the type-pinned lock-skip flag is set, refund the attempt (never count it toward the cap or the insufficient-progress blocker), and when the turn cannot proceed (provider already proved the request does not fit) end it with a soft compression_deferred result — distinct from compression_exhausted — so the gateway keeps the session intact and the next message retries after the concurrent compressor finishes. The new compression_skipped_due_to_lock() reader is type-pinned (is True or isinstance(str)) per the MagicMock auto-attribute rule, and compress_context() now also clears the signal at the very top of every attempt (per-attempt state rule, NousResearch#58629/NousResearch#69853) so a stale value can never make a later breaker/codex no-op look like lock contention. Salvaged from PR NousResearch#49874; rebuilt on main's NousResearch#69870 _compression_skipped_due_to_lock signal instead of the PR's parallel _compression_deferred_by_lock triple.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Manual
/compressduring a concurrent compression now tells the user the real reason for the no-op — including who holds the lock — instead of the misleading "No changes from compression" text. Root cause: the lock-acquisition failure path incompress_contextreturns messages unchanged with no signal, so every manual-compress feedback surface read it as a genuine no-op (issue #57631).Changes
agent/conversation_compression.py: lock-skip setsagent._compression_skipped_due_to_lock(holder string when confirmed,Trueotherwise); cleared at entry of every call so a stale auto-compress signal can't leak into a later successful manual/compress.agent/manual_compression_feedback.py: newdescribe_compression_lock_skip()— single source of truth for the wording. Classification fix over the original PR: a confirmed holder string says "already in progress (holder: …)";True/None(acquisition failed buthermes_state.try_acquire_compression_lockcatchessqlite3.Errorinternally and returnsFalse, so no concurrent compressor is proven) says "could not acquire this session's compression lock … or the lock check failed" instead of falsely asserting another compression is running.cli.py_manual_compress: prints the lock-skip message, clears the signal, and discards the deferred context-engine notification (finalize(committed=False), fix(compression): notify the context engine only after the session commit succeeds #69324 contract) before returning.gateway/slash_commands.py/compress: returns the shared wording (deferred notification discarded by the existingfinally).tui_gateway/server.py:_compress_session_historyraisesCompressionLockHeld(and discards the deferred notification first); all three in-process consumers handle it —session.compressRPC (lock_held: truepayload), thecommand.dispatchcompress branch (new — previously fell into the generic "compress failed: …" error), and theslash.execmirror. Compute-host isolated routes inherit via_mirror_slash_side_effectsrunning inside the host child.VISIBLE_COMPRESSION_MESSAGESnoise-filter carve-outs for both wordings, and MagicMock signal opt-outs for sibling tests added on main after the original PR.Validation
/compresswhile another compression holds the lockFalse)existing or Truedefect)command.dispatchcompress during lock hold5009 compress failed: Compression lock heldTargeted tests:
bash scripts/run_tests.sh tests/ -q -k 'lock and compress'→ 80 passed, 0 failed; manual-compress feedback suite (13 files incl.test_manual_compress.py,test_compress_command.py,test_compress_lock_skip.py,test_telegram_noise_filter.py,test_tui_gateway_server.py,test_compression_concurrent_fork.py) → 1190 passed, 0 failed.Credit
Salvaged from #57634 by @0xLeathery. Fixes #57631.
Infographic