fix(gateway): bound the hygiene-compression turn-hold (salvage of #90845) - #92318
kshitijk4poor merged 4 commits into
Conversation
This is careful concurrency work: the bounded wait re-slices against the remaining turn-hold budget so a continuously-streaming worker can't skip past the check ( Two notes:
|
|
@kshitijk4poor — thanks for salvaging this. Both reviewer points above are addressed in the patch below (applies cleanly on your branch head Point 1 — bounded overshoot: documented at the await site. The turn can exceed Point 2 — un-localized bubble: the From 4353fccfffada2475f4ba76de74d2c815101c170 Mon Sep 17 00:00:00 2001
From: Osham Wahab <osham@users.noreply.github.com>
Date: Sun, 23 Aug 2026 17:14:06 +0000
Subject: [PATCH] fix(gateway): document turn-hold commit overshoot + route
deferred-notice through i18n (#92318 review)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Reviewer feedback on #92318:
1. Bounded overshoot in the commit-in-flight branch is now documented at
the await site — the turn can exceed _hyg_max_turn_hold_seconds by up
to commit duration, and aborting mid-commit would corrupt the
message-store transaction. Prevents a future 'fix' into mid-commit
cancellation.
2. The user-facing 'Context compression deferred' bubble now routes
through t() as gateway.compress.turnhold_deferred (en.yaml entry
added), coordinating with the i18n surface expanding in #92338 so
non-English users don't get hardcoded English copy.
---
gateway/run.py | 15 +++++++++++----
locales/en.yaml | 1 +
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/gateway/run.py b/gateway/run.py
index a2930483c..bc5c97c66 100644
--- a/gateway/run.py
+++ b/gateway/run.py
@@ -19878,6 +19878,15 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
if _cancelled is None:
await asyncio.sleep(0.025)
if not _cancelled:
+ # NOTE: bounded overshoot by design.
+ # The turn can be held past
+ # _hyg_max_turn_hold_seconds by up to the
+ # commit duration (summary apply + storage
+ # write). Aborting mid-commit would corrupt
+ # the message-store transaction — the
+ # overshoot is the cheaper failure mode.
+ # Do NOT "fix" this into a mid-commit
+ # cancellation.
_compressed, _ = await _hyg_future
else:
_hyg_commit_fence.release_cancelled_compression_lock()
@@ -19921,10 +19930,8 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
session_entry.session_id,
time.monotonic() - _hyg_wait_started,
)
- _turnhold_msg = (
- "ℹ️ Context compression deferred — "
- "summary still streaming. "
- "Continuing without compression this turn."
+ _turnhold_msg = t(
+ "gateway.compress.turnhold_deferred"
)
try:
_adapter = self._adapter_for_source(source)
diff --git a/locales/en.yaml b/locales/en.yaml
index b395fb99a..ade6e1f98 100644
--- a/locales/en.yaml
+++ b/locales/en.yaml
@@ -113,6 +113,7 @@ gateway:
summary_failed: "⚠️ Summary generation failed ({error}). {count} historical message(s) were removed and replaced with a placeholder; earlier context is no longer recoverable. Consider checking your auxiliary.compression model configuration."
aborted: "⚠️ Compression aborted ({error}). No messages were dropped — conversation is unchanged. Run /compress to retry, /reset for a clean session, or check your auxiliary.compression model configuration."
aux_failed: "ℹ️ Configured compression model `{model}` failed ({error}). Recovered using your main model — context is intact — but you may want to check `auxiliary.compression.model` in config.yaml."
+ turnhold_deferred: "ℹ️ Context compression deferred — summary still streaming. Continuing without compression this turn."
failed: "Compression failed: {error}"
debug:
--
2.47.3
If it's easier, the same commit lives on a branch — happy to open a PR against your fork, just say the word. |
28d21e2 to
38dd81c
Compare
|
Updated — all three review threads closed in one push:
@Enough1122's both points are now addressed on-branch. Arming auto-merge (rebase). |
…summary cannot freeze the turn Session hygiene auto-compression runs inline on the incoming-message path and awaits the summary worker with a progress-aware inactivity budget (hygiene_timeout_seconds) that extends up to hygiene_total_ceiling_seconds (default 600s). A summary model that keeps streaming tokens keeps resetting the inactivity slice, so the wait can stretch toward the ceiling while zero bytes reach the user — chat transports (Telegram ~30s idle-timeout) drop the connection and the turn appears frozen, even though the gateway is healthy. Add hygiene_max_turn_hold_seconds (default 10), a turn-hold budget that caps the wall-clock the incoming message waits on hygiene compression. The wait slice is additionally capped at the remaining budget so the budget is re-evaluated even when the worker keeps the inactivity slice large. On exceeding the budget the gateway abandons the inline wait and proceeds on the uncompressed transcript via the existing timeout path, which revokes the worker's commit admission (CompressionCommitFence) and defers cleanup — so a stale compression finishing later can never overwrite the turns appended after the wait was abandoned. Well under the typical transport idle-timeout, this guarantees the message is answered promptly while the detached compression completes in the background. Configurable via compression.hygiene_max_turn_hold_seconds. Adds a regression test: a worker that streams progress continuously (so the inactivity slice never fires) must be abandoned once it exceeds the turn-hold budget, the turn proceeds uncompressed, and the stale commit is fenced (no session mutation, role alternation intact).
Introduce HygieneTurnHoldExceeded exception so turn-hold budget expiry no longer collapses into the generic asyncio.TimeoutError handler. - Add HygieneTurnHoldExceeded exception (availability boundary, not a failure) - Add dedicated handler: stamps AGENT_COMPRESSION_TURNHOLD provenance, sends deferral notice, does NOT increment failure cooldown - Preserve NousResearch#87011 contract: idle timeout still sends 'no output' message and takes failure path - Add behavior witnesses: turn-hold ≠ idle timeout, cooldown untouched Fixes semantic boundary collapse flagged in PR NousResearch#90845 review.
…er on turn-hold abandonment Review follow-ups on the salvaged NousResearch#90845: - hygiene_max_turn_hold_seconds registered in config_defaults next to its sibling hygiene knobs (run.py already read it; the key was undiscoverable). - Turn-hold abandonment now records a flat 60s retry-after via the existing cooldown column. Without it, sustained traffic re-spawned, held, and cancelled a fresh compressor on EVERY turn — a per-turn summary-model token burn that never commits. Deliberately outside the x1/x3/x9 failure ladder: the compressor is healthy, so the failure streak must not advance (witness updated to assert exactly that boundary: no streak increment, flat <=120s spacing, turn-hold reason).
…tice through i18n (NousResearch#92318 review) Reviewer feedback on NousResearch#92318: 1. Bounded overshoot in the commit-in-flight branch is now documented at the await site — the turn can exceed _hyg_max_turn_hold_seconds by up to commit duration, and aborting mid-commit would corrupt the message-store transaction. Prevents a future 'fix' into mid-commit cancellation. 2. The user-facing 'Context compression deferred' bubble now routes through t() as gateway.compress.turnhold_deferred (en.yaml entry added), coordinating with the i18n surface expanding in NousResearch#92338 so non-English users don't get hardcoded English copy.
|
CI failure root-caused and fixed — my miss, good catch by the parity suite: What failed: Fix (amended into the i18n commit so every commit stays green): the key added to all 16 non-English catalogs with real translations (not English copies) — zh, zh-hant, ja, ko, de, es, fr, it, pt, ru, uk, tr, af, hu, ar, ga — following each catalog's existing Verified: Re-arming auto-merge (rebase). |
38dd81c to
9f6223d
Compare
… thinking-model summaries are adopted, not burned The 10s hygiene_max_turn_hold_seconds budget (#92318) releases the arriving user turn while the summary model is still streaming. For thinking summary models (DeepSeek-V4-Flash etc.) whose reasoning prefix alone exceeds 10s, the abandonment path ALWAYS cancelled the commit fence — 100% of the summary attempt (including the full thinking prefix) was discarded on every turn, permanently disabling auto-compression while paying the summary model 10s of thinking per turn, and the flat 60s retry-after then blocked the agent-side preflight from a fresh chance. Structural fix (maintainer-chosen direction in #97963): decouple the turn from the compression instead of holding the turn longer or making the hold progress-aware (which would reintroduce the #90845 frozen-turn bug): - CompressionCommitFence gains mark_commit_watermark_fenced() / commit_watermark_fenced; compress_context marks the fence right after capturing get_active_message_watermark() under the durable compression lock (#75316/#87484) — the property that makes a LATE commit safe: rows appended after compression start survive both commit paths verbatim as cloned concurrent tail (archive_and_compact watermark= and publish_compression_child watermark/watermark_ceiling). - gateway hygiene turn-hold handler: when the fence is watermark-fenced, the detached worker (already kept alive via _defer_agent_cleanup_until_future_done) KEEPS its commit admission; the user's turn proceeds on the uncompressed transcript at the same 10s budget, and the summary is adopted at the worker's own watermark-fenced commit boundary. Unfenced workers are cancelled exactly as before — never worse than the status quo. - No retry-after is armed while the kept-admission attempt runs (it would block preflight adoption via the same-session cooldown); re-attempt spacing is covered by the durable compression lock (_session_has_compression_in_flight). If the worker ends WITHOUT committing, a done-callback restores the flat non-escalating 60s retry-after; a successful adoption resets the hygiene failure streak. The streak never advances for a deferral either way. - Docs: configuration.md hygiene_max_turn_hold_seconds one-liner updated to describe deferred adoption and the thinking-model case; config_defaults.py comment updated. Knob stays config.yaml-only. Invariants preserved: - 10s user-latency cap stays hard (#90845/#92318): test_session_hygiene_turn_hold_budget_abandons_streaming_wait passes UNMODIFIED (its worker is not watermark-fenced, so it pins the cancel path through the public surface). - Stale-clobber impossible: adoption only rides commits bounded by the start watermark; the fence still gates admission and unfenced/late results are discarded. New regression tests (tests/gateway/test_session_hygiene_turnhold_adoption.py): - watermark-fenced worker keeps admission, late summary is committed, turn still released at the budget, no cooldown while running, streak reset on adoption; - kept-admission worker that ends without committing restores the flat turn-hold retry-after (<=120s, names turn-hold, streak untouched); - unfenced worker still cancelled and discarded (status quo). Sabotage-verified: disabling the keep-admission branch fails the two new adoption tests and leaves the unfenced-cancel test green. Fixes #97963
… thinking-model summaries are adopted, not burned The 10s hygiene_max_turn_hold_seconds budget (#92318) releases the arriving user turn while the summary model is still streaming. For thinking summary models (DeepSeek-V4-Flash etc.) whose reasoning prefix alone exceeds 10s, the abandonment path ALWAYS cancelled the commit fence — 100% of the summary attempt (including the full thinking prefix) was discarded on every turn, permanently disabling auto-compression while paying the summary model 10s of thinking per turn, and the flat 60s retry-after then blocked the agent-side preflight from a fresh chance. Structural fix (maintainer-chosen direction in #97963): decouple the turn from the compression instead of holding the turn longer or making the hold progress-aware (which would reintroduce the #90845 frozen-turn bug): - CompressionCommitFence gains mark_commit_watermark_fenced() / commit_watermark_fenced; compress_context marks the fence right after capturing get_active_message_watermark() under the durable compression lock (#75316/#87484) — the property that makes a LATE commit safe: rows appended after compression start survive both commit paths verbatim as cloned concurrent tail (archive_and_compact watermark= and publish_compression_child watermark/watermark_ceiling). - gateway hygiene turn-hold handler: when the fence is watermark-fenced, the detached worker (already kept alive via _defer_agent_cleanup_until_future_done) KEEPS its commit admission; the user's turn proceeds on the uncompressed transcript at the same 10s budget, and the summary is adopted at the worker's own watermark-fenced commit boundary. Unfenced workers are cancelled exactly as before — never worse than the status quo. - No retry-after is armed while the kept-admission attempt runs (it would block preflight adoption via the same-session cooldown); re-attempt spacing is covered by the durable compression lock (_session_has_compression_in_flight). If the worker ends WITHOUT committing, a done-callback restores the flat non-escalating 60s retry-after; a successful adoption resets the hygiene failure streak. The streak never advances for a deferral either way. - Docs: configuration.md hygiene_max_turn_hold_seconds one-liner updated to describe deferred adoption and the thinking-model case; config_defaults.py comment updated. Knob stays config.yaml-only. Invariants preserved: - 10s user-latency cap stays hard (#90845/#92318): test_session_hygiene_turn_hold_budget_abandons_streaming_wait passes UNMODIFIED (its worker is not watermark-fenced, so it pins the cancel path through the public surface). - Stale-clobber impossible: adoption only rides commits bounded by the start watermark; the fence still gates admission and unfenced/late results are discarded. New regression tests (tests/gateway/test_session_hygiene_turnhold_adoption.py): - watermark-fenced worker keeps admission, late summary is committed, turn still released at the budget, no cooldown while running, streak reset on adoption; - kept-admission worker that ends without committing restores the flat turn-hold retry-after (<=120s, names turn-hold, streak untouched); - unfenced worker still cancelled and discarded (status quo). Sabotage-verified: disabling the keep-admission branch fails the two new adoption tests and leaves the unfenced-cancel test green. Fixes #97963
… thinking-model summaries are adopted, not burned The 10s hygiene_max_turn_hold_seconds budget (NousResearch#92318) releases the arriving user turn while the summary model is still streaming. For thinking summary models (DeepSeek-V4-Flash etc.) whose reasoning prefix alone exceeds 10s, the abandonment path ALWAYS cancelled the commit fence — 100% of the summary attempt (including the full thinking prefix) was discarded on every turn, permanently disabling auto-compression while paying the summary model 10s of thinking per turn, and the flat 60s retry-after then blocked the agent-side preflight from a fresh chance. Structural fix (maintainer-chosen direction in NousResearch#97963): decouple the turn from the compression instead of holding the turn longer or making the hold progress-aware (which would reintroduce the NousResearch#90845 frozen-turn bug): - CompressionCommitFence gains mark_commit_watermark_fenced() / commit_watermark_fenced; compress_context marks the fence right after capturing get_active_message_watermark() under the durable compression lock (NousResearch#75316/NousResearch#87484) — the property that makes a LATE commit safe: rows appended after compression start survive both commit paths verbatim as cloned concurrent tail (archive_and_compact watermark= and publish_compression_child watermark/watermark_ceiling). - gateway hygiene turn-hold handler: when the fence is watermark-fenced, the detached worker (already kept alive via _defer_agent_cleanup_until_future_done) KEEPS its commit admission; the user's turn proceeds on the uncompressed transcript at the same 10s budget, and the summary is adopted at the worker's own watermark-fenced commit boundary. Unfenced workers are cancelled exactly as before — never worse than the status quo. - No retry-after is armed while the kept-admission attempt runs (it would block preflight adoption via the same-session cooldown); re-attempt spacing is covered by the durable compression lock (_session_has_compression_in_flight). If the worker ends WITHOUT committing, a done-callback restores the flat non-escalating 60s retry-after; a successful adoption resets the hygiene failure streak. The streak never advances for a deferral either way. - Docs: configuration.md hygiene_max_turn_hold_seconds one-liner updated to describe deferred adoption and the thinking-model case; config_defaults.py comment updated. Knob stays config.yaml-only. Invariants preserved: - 10s user-latency cap stays hard (NousResearch#90845/NousResearch#92318): test_session_hygiene_turn_hold_budget_abandons_streaming_wait passes UNMODIFIED (its worker is not watermark-fenced, so it pins the cancel path through the public surface). - Stale-clobber impossible: adoption only rides commits bounded by the start watermark; the fence still gates admission and unfenced/late results are discarded. New regression tests (tests/gateway/test_session_hygiene_turnhold_adoption.py): - watermark-fenced worker keeps admission, late summary is committed, turn still released at the budget, no cooldown while running, streak reset on adoption; - kept-admission worker that ends without committing restores the flat turn-hold retry-after (<=120s, names turn-hold, streak untouched); - unfenced worker still cancelled and discarded (status quo). Sabotage-verified: disabling the keep-admission branch fails the two new adoption tests and leaves the unfenced-cancel test green. Fixes NousResearch#97963 (cherry picked from commit 9de9d76)
…tice through i18n (NousResearch#92318 review) Reviewer feedback on NousResearch#92318: 1. Bounded overshoot in the commit-in-flight branch is now documented at the await site — the turn can exceed _hyg_max_turn_hold_seconds by up to commit duration, and aborting mid-commit would corrupt the message-store transaction. Prevents a future 'fix' into mid-commit cancellation. 2. The user-facing 'Context compression deferred' bubble now routes through t() as gateway.compress.turnhold_deferred (en.yaml entry added), coordinating with the i18n surface expanding in NousResearch#92338 so non-English users don't get hardcoded English copy.
… thinking-model summaries are adopted, not burned The 10s hygiene_max_turn_hold_seconds budget (NousResearch#92318) releases the arriving user turn while the summary model is still streaming. For thinking summary models (DeepSeek-V4-Flash etc.) whose reasoning prefix alone exceeds 10s, the abandonment path ALWAYS cancelled the commit fence — 100% of the summary attempt (including the full thinking prefix) was discarded on every turn, permanently disabling auto-compression while paying the summary model 10s of thinking per turn, and the flat 60s retry-after then blocked the agent-side preflight from a fresh chance. Structural fix (maintainer-chosen direction in NousResearch#97963): decouple the turn from the compression instead of holding the turn longer or making the hold progress-aware (which would reintroduce the NousResearch#90845 frozen-turn bug): - CompressionCommitFence gains mark_commit_watermark_fenced() / commit_watermark_fenced; compress_context marks the fence right after capturing get_active_message_watermark() under the durable compression lock (NousResearch#75316/NousResearch#87484) — the property that makes a LATE commit safe: rows appended after compression start survive both commit paths verbatim as cloned concurrent tail (archive_and_compact watermark= and publish_compression_child watermark/watermark_ceiling). - gateway hygiene turn-hold handler: when the fence is watermark-fenced, the detached worker (already kept alive via _defer_agent_cleanup_until_future_done) KEEPS its commit admission; the user's turn proceeds on the uncompressed transcript at the same 10s budget, and the summary is adopted at the worker's own watermark-fenced commit boundary. Unfenced workers are cancelled exactly as before — never worse than the status quo. - No retry-after is armed while the kept-admission attempt runs (it would block preflight adoption via the same-session cooldown); re-attempt spacing is covered by the durable compression lock (_session_has_compression_in_flight). If the worker ends WITHOUT committing, a done-callback restores the flat non-escalating 60s retry-after; a successful adoption resets the hygiene failure streak. The streak never advances for a deferral either way. - Docs: configuration.md hygiene_max_turn_hold_seconds one-liner updated to describe deferred adoption and the thinking-model case; config_defaults.py comment updated. Knob stays config.yaml-only. Invariants preserved: - 10s user-latency cap stays hard (NousResearch#90845/NousResearch#92318): test_session_hygiene_turn_hold_budget_abandons_streaming_wait passes UNMODIFIED (its worker is not watermark-fenced, so it pins the cancel path through the public surface). - Stale-clobber impossible: adoption only rides commits bounded by the start watermark; the fence still gates admission and unfenced/late results are discarded. New regression tests (tests/gateway/test_session_hygiene_turnhold_adoption.py): - watermark-fenced worker keeps admission, late summary is committed, turn still released at the budget, no cooldown while running, streak reset on adoption; - kept-admission worker that ends without committing restores the flat turn-hold retry-after (<=120s, names turn-hold, streak untouched); - unfenced worker still cancelled and discarded (status quo). Sabotage-verified: disabling the keep-admission branch fails the two new adoption tests and leaves the unfenced-cancel test green. Fixes NousResearch#97963
Summary
A hygiene compression whose summary was still streaming could hold an arriving user turn indefinitely — the user saw a frozen bot. Salvage of #90845 by Machan-Army (2 commits, authorship preserved): the turn-hold is bounded (default 10s, now configurable); on expiry the turn proceeds uncompressed and the still-streaming summary is fence-protected in the background (a stale commit can never clobber newer turns).
Changes
HygieneTurnHoldExceededpath — AGENT_COMPRESSION_TURNHOLD provenance, deferral notice, NO failure-cooldown ladder (the fix(agent): do not let hygiene idle timeouts block in-agent compression #87011 "timed out/no output" contract stays truthful for real idle timeouts)compression.hygiene_max_turn_hold_secondsregistered in config_defaults (run.py already read it; the key was undiscoverable)Validation
Closes #90845.
Infographic