-
Notifications
You must be signed in to change notification settings - Fork 59
fix(providers): guard compaction against token estimate drift #507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | |||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -94,6 +94,25 @@ async def _estimate_context_tokens( | ||||||||||||||||
| ) | |||||||||||||||||
|
|
|||||||||||||||||
|
|
|||||||||||||||||
| def _count_utf8_bytes(text: str) -> int: | |||||||||||||||||
| """Return a one-byte-per-token safety bound for token-dense text.""" | |||||||||||||||||
| return len(text.encode("utf-8")) | |||||||||||||||||
|
|
|||||||||||||||||
|
|
|||||||||||||||||
| def _estimate_conservative_context_tokens( | |||||||||||||||||
| messages: list[ModelMessage], | |||||||||||||||||
| model_request_parameters: ModelRequestParameters | None, | |||||||||||||||||
| ) -> int: | |||||||||||||||||
| """Estimate context with a one-byte-per-token safety bound.""" | |||||||||||||||||
| from pydantic_ai_harness.compaction import estimate_context_tokens | |||||||||||||||||
|
|
|||||||||||||||||
| return estimate_context_tokens( | |||||||||||||||||
| messages, | |||||||||||||||||
| tokenizer=_count_utf8_bytes, | |||||||||||||||||
| model_request_parameters=model_request_parameters, | |||||||||||||||||
| ) | |||||||||||||||||
|
|
|||||||||||||||||
|
|
|||||||||||||||||
| def _estimate_after_compaction_tokens( | |||||||||||||||||
| before_messages: list[ModelMessage], | |||||||||||||||||
| after_messages: list[ModelMessage], | |||||||||||||||||
|
|
@@ -254,28 +273,59 @@ async def before_model_request( | ||||||||||||||||
| if self._disabled: | |||||||||||||||||
| return request_context | |||||||||||||||||
|
|
|||||||||||||||||
| # Zone (a): gate measurement. A broken estimate says nothing about | |||||||||||||||||
| # the compaction path, so this warns and skips compaction for this | |||||||||||||||||
| # request only — no errored event, no disable latch. | |||||||||||||||||
| before_messages = list(request_context.messages) | |||||||||||||||||
| conservative_estimate: int | None = None | |||||||||||||||||
| try: | |||||||||||||||||
| before_messages = list(request_context.messages) | |||||||||||||||||
| before_estimate = await _estimate_context_tokens( | |||||||||||||||||
| before_messages, | |||||||||||||||||
| request_context.model_request_parameters, | |||||||||||||||||
| ) | |||||||||||||||||
| except Exception: # noqa: BLE001 - estimation must never fail the run | |||||||||||||||||
| logger.warning( | |||||||||||||||||
| "Compaction gate measurement failed for agent %r; " | |||||||||||||||||
| "skipping compaction for this request.", | |||||||||||||||||
| "Compaction gate measurement failed for agent %r; using the conservative fallback.", | |||||||||||||||||
| self._config.agent_name, | |||||||||||||||||
| exc_info=True, | |||||||||||||||||
| ) | |||||||||||||||||
| try: | |||||||||||||||||
| conservative_estimate = _estimate_conservative_context_tokens( | |||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RECOMMENDED The conservative estimator isn't independent of the primary, so the fallback is unreachable for realistic failures.
I verified this by patching the shared harness function the way a real estimator bug would fail: both warnings fire, compaction is skipped, no events, no latch — byte-for-byte the pre-PR outcome, after fifteen extra lines and a second full traversal. Both new tests hide this by patching Suggested fix: either drop the fallback branch and restore the simple skip, or make it genuinely independent — walk
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — the old pair differed only in the The fallback is now genuinely independent in e8490bf: |
|||||||||||||||||
| before_messages, | |||||||||||||||||
| request_context.model_request_parameters, | |||||||||||||||||
| ) | |||||||||||||||||
| except Exception: # noqa: BLE001 - estimation must never fail the run | |||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RECOMMENDED Both new error branches are entirely uncovered, and the only surviving documented behaviour is untested. Branch coverage on this module is 96%, missing exactly lines 294-301 and 309-310 — the two error paths this PR introduces. Lines 294-301 (both estimators failed → return unchanged, no event, no latch) is now the only path that still behaves the way the class docstring describes, and the only remaining safety net against an exception escaping into the model request path. Lines 309-315 (primary succeeded, conservative raised) silently disarms the window guard for that request; if it were wrong — say Separately, neither degradation is visible outside stderr. Conductor installs no logging handlers in Suggested fix: add both tests (patch
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both branches are now covered in e8490bf, and the visibility gap is closed the way you suggested:
|
|||||||||||||||||
| logger.warning( | |||||||||||||||||
| "Compaction fallback measurement failed for agent %r; " | |||||||||||||||||
| "skipping compaction for this request.", | |||||||||||||||||
| self._config.agent_name, | |||||||||||||||||
| exc_info=True, | |||||||||||||||||
| ) | |||||||||||||||||
| return request_context | |||||||||||||||||
| before_estimate = conservative_estimate | |||||||||||||||||
| else: | |||||||||||||||||
| try: | |||||||||||||||||
| conservative_estimate = _estimate_conservative_context_tokens( | |||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RECOMMENDED The duplicated conservative-estimate handler makes control flow do the work of a value. The One side effect worth flagging: the second copy raises its warning from inside an active exception handler, so its One caution on the neighbouring cleanup — the apparently redundant Suggested fix: let both estimates be primary_estimate: int | None = None
try:
primary_estimate = await _estimate_context_tokens(...)
except Exception: # noqa: BLE001 - estimation must never fail the run
logger.warning("Compaction gate measurement failed for agent %r; "
"using the conservative fallback.", ..., exc_info=True)
conservative_estimate: int | None = None
try:
conservative_estimate = _estimate_conservative_context_tokens(...)
except Exception: # noqa: BLE001 - estimation must never fail the run
outcome = ("skipping compaction for this request." if primary_estimate is None
else "using the primary estimate.")
logger.warning("Compaction safety measurement failed for agent %r; %s",
self._config.agent_name, outcome, exc_info=True)
if primary_estimate is not None:
before_estimate = primary_estimate
elif conservative_estimate is not None:
before_estimate = conservative_estimate
else:
return request_contextThen fold the window condition directly into the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restructured in e8490bf along the lines you sketched: both estimates are resolved as One deliberate deviation from the sketch: the two arms no longer call the same function. The |
|||||||||||||||||
| before_messages, | |||||||||||||||||
| request_context.model_request_parameters, | |||||||||||||||||
| ) | |||||||||||||||||
| except Exception: # noqa: BLE001 - primary estimate remains usable | |||||||||||||||||
| logger.warning( | |||||||||||||||||
| "Compaction safety measurement failed for agent %r; " | |||||||||||||||||
| "using the primary estimate.", | |||||||||||||||||
| self._config.agent_name, | |||||||||||||||||
| exc_info=True, | |||||||||||||||||
| ) | |||||||||||||||||
|
|
|||||||||||||||||
| # The primary estimate drives the reserve-based trigger. The byte bound | |||||||||||||||||
| # only guards the hard window, avoiding premature compaction of ordinary | |||||||||||||||||
| # text while catching suffixes the four-characters heuristic undercounts. | |||||||||||||||||
| exceeds_trigger = before_estimate > self._config.trigger_tokens | |||||||||||||||||
| may_exceed_window = ( | |||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BLOCKING The window guard is a no-op whenever the primary estimate is at or below When So the new branch only ever compacts in the narrow band This is anti-correlated with the bug being fixed. Content the heuristic undercounts is exactly what pushes the primary estimate down into the no-op region. I verified this against the real Suggested fix: the bound has to reach the escalation loop, not just the outer gate. Either build a second orchestrator over the same tier wrappers with the byte tokenizer and delegate to it when the window bound is what fired: tiered_bytes = TieredCompaction(
tiers=[clear_tier, summarize_tier, slide_tier],
target_tokens=config.target_tokens,
tokenizer=_count_utf8_bytes, # baseline and reclaim in the same unit
)or bypass
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and reproduced before changing anything: under the production-resolved plan ( Fixed in e8490bf by taking your second option: on the guard path the wrapper now drives the tier chain directly ( Your "at minimum" is covered as well: when the guard fired but the history is unchanged, the wrapper logs at error, and the complete event carries |
|||||||||||||||||
| conservative_estimate is not None | |||||||||||||||||
| and conservative_estimate >= self._config.window_tokens | |||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BLOCKING Byte bound fires at roughly a quarter of the window on ordinary text, producing a per-request no-op loop and a spurious user-facing warning. A UTF-8 byte count runs about 4x a real token count for English prose, and the guard fires at Measured against the real stack with the project's own resolver ( Since 54,000 is below The anchored path is milder but still reachable: the byte overcount applies to the post-anchor suffix, so the guard fires below the trigger once that suffix reaches about a third of the reserve. Worth noting: the PR's own reported evidence (primary ~106k, bytes >200k) reproduces exactly from plain ASCII prose with a ~74.5k anchor — it isn't by itself evidence of token density. Suggested fix: a raw byte count overshoots real tokens by ~4x for English, while the heuristic's undercount on dense content is only ~1.3-2x — the correction is bigger than the error it's correcting for. Replace the absolute byte-vs-window comparison with something calibrated: a real tokenizer for the post-anchor suffix, or a density signal (bytes-per-character, non-ASCII fraction) that only escalates the estimate for suffixes that are actually dense. Whatever the mechanism, add the negative test that's missing today, because it fails against the PR as written: @pytest.mark.asyncio
async def test_ordinary_prose_below_trigger_is_not_compacted(self) -> None:
events: list[tuple[str, dict[str, Any]]] = []
cfg = _make_config( # window_tokens defaults to 200_000
trigger_tokens=96_000, target_tokens=86_000,
event_callback=lambda t, d: events.append((t, d)),
)
capability = build_tiered_compaction(cfg)
prose = "The quick brown fox jumps over the lazy dog and then writes a "
messages = [
ModelRequest(parts=[UserPromptPart(content=(prose * 70)[:4_000])])
for _ in range(50)
] # 200_000 bytes ~= 50_000 real tokens
request_context = _request_context_with_messages(messages)
result = await capability.before_model_request(_make_run_context(), request_context)
assert len(result.messages) == len(messages)
assert not eventsAnd rewrite the comment at lines 317-319 so it states the real trade-off instead of ruling out the failure mode it actually introduces.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — measured on the previous revision: 50 ordinary prose messages of 4,000 characters each gives primary 50,000 and conservative 200,000, so the guard tripped at 25% of the window on plain English, exactly as you calculated. The point about the PR's own evidence (primary ~106k / bytes >200k) reproducing from plain ASCII with a ~74.5k anchor was also correct. The raw byte count is gone in e8490bf. The guard now uses a density-calibrated bound ( Your negative test is added nearly verbatim as |
|||||||||||||||||
| ) | |||||||||||||||||
| if not exceeds_trigger and not may_exceed_window: | |||||||||||||||||
| return request_context | |||||||||||||||||
|
|
|||||||||||||||||
| # Gate on the token estimate, not the message count: one large | |||||||||||||||||
| # prompt can exceed the trigger with nothing to drop. | |||||||||||||||||
| if before_estimate <= self._config.trigger_tokens: | |||||||||||||||||
| return request_context | |||||||||||||||||
| if conservative_estimate is not None and may_exceed_window: | |||||||||||||||||
| before_estimate = max(before_estimate, conservative_estimate) | |||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BLOCKING
Measured on the PR's own new test scenario, where compaction genuinely succeeds and drops 10 of 31 messages:
Because Raising Suggested fix: drop the promotion and keep window_guard_tripped = (
conservative_estimate is not None
and conservative_estimate >= self._config.window_tokens
)
if not window_guard_tripped and before_estimate <= self._config.trigger_tokens:
return request_contextIf the safety bound is useful for diagnostics, surface it as its own explicitly-named field on the start event (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — on the PR's own scenario the emitted payload read The promotion is removed in e8490bf. The genuine failure mode you identified — a guard compaction that cannot get back under the window — is now reported honestly via |
|||||||||||||||||
|
|
|||||||||||||||||
| self._on_before(estimate=before_estimate, messages_before=len(before_messages)) | |||||||||||||||||
| for tier in self._tier_wrappers: | |||||||||||||||||
|
|
|||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -332,6 +332,60 @@ async def test_gate_measures_once_via_wrapper_estimate(self) -> None: | |
| assert len(result.messages) == 1 | ||
|
|
||
|
|
||
| class TestKnownWindowSafety: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RECOMMENDED No non-ASCII coverage: replacing the UTF-8 encode with a character count passes the whole suite. The stated justification for a byte bound is content the four-chars-per-token heuristic undercounts — CJK, emoji, non-Latin scripts, base64. None of it is tested. Mutating The fixture in the new test compounds this: The Suggested fix: add a direct unit test for the encoder and a boundary test for the comparison: def test_conservative_estimator_counts_utf8_bytes_not_characters(self) -> None:
messages = [ModelRequest(parts=[UserPromptPart(content="日本語" * 1_000)])]
assert _estimate_conservative_context_tokens(
messages, ModelRequestParameters()
) == 9_000and rebuild the token-dense fixture from content that's genuinely dense relative to the heuristic — CJK text or base64 — rather than repeated punctuation, so the scenario matches the failure the PR describes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both gaps are closed in e8490bf:
|
||
| """Requirement: known context windows are hard pre-request boundaries.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_token_dense_suffix_compacts_before_known_window_overflow(self) -> None: | ||
| # Requirement: provider usage plus token-dense suffix growth must compact before | ||
| # the next request can exceed the known context window. | ||
| capability = build_tiered_compaction( | ||
| _make_config(trigger_tokens=121_000, target_tokens=100_000) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BLOCKING The regression test hard-codes a
That 10,000-token difference is decisive, because the inner The test passes only because it picks a target just below the scenario's primary estimate of 106,004. Under the configuration Conductor actually ships, the reported bug isn't fixed — the guard fires, the inner strategy declines, and the oversized request goes out unchanged behind a success-shaped The same hard-coded pair shows up again at line 373 for the second new test. Suggested fix: derive the test configuration from the real resolver so a regression test can't pass against an unreachable config: plan = resolve_compaction_plan(
window=200_000, output_limit=64_000, tool_buffer=15_000
)
capability = build_tiered_compaction(
_make_config(
trigger_tokens=plan.trigger_tokens,
target_tokens=plan.target_tokens,
)
)Then re-run the scenario. It'll fail — that's the correct signal — and resolve it as part of the delegation fix above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — The regression test now derives its configuration exactly as you suggested, from |
||
| ) | ||
| messages: list[Any] = [ | ||
| ModelResponse( | ||
| parts=[TextPart(content="compaction complete")], | ||
| usage=RequestUsage(input_tokens=74_499, output_tokens=0), | ||
| ) | ||
| ] | ||
| for index in range(30): | ||
| messages.append( | ||
| ModelRequest(parts=[UserPromptPart(content=f"turn-{index}-" + "!" * 4_193)]) | ||
| ) | ||
| request_context = _request_context_with_messages(messages) | ||
|
|
||
| result = await capability.before_model_request(_make_run_context(), request_context) | ||
|
|
||
| assert len(result.messages) < len(messages) | ||
| assert len(result.messages) == 21 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RECOMMENDED
The class states its requirement as "known context windows are hard pre-request boundaries," but It also hides which tier actually ran. The summarizing tier raises The actual safety post-condition is never checked at all. The compacted result measures Suggested fix: assert len(result.messages) < len(messages)
assert (
_estimate_conservative_context_tokens(
list(result.messages), request_context.model_request_parameters
)
< cfg.window_tokens
), "compaction must bring the byte bound back under the known window"Also assert
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All three points addressed in e8490bf:
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_estimator_failure_uses_safe_fallback_before_large_request(self) -> None: | ||
| # Requirement: a failed primary estimate must not bypass compaction when an | ||
| # independent conservative estimate shows the request can exceed the known window. | ||
| inner = AsyncMock() | ||
| compacted = _request_context_with_messages( | ||
| [ModelRequest(parts=[UserPromptPart(content="compacted")])] | ||
| ) | ||
| inner.before_model_request = AsyncMock(return_value=compacted) | ||
| capability = _FailOpenCompactionWrapper( | ||
| inner, | ||
| config=_make_config(trigger_tokens=121_000, target_tokens=100_000), | ||
| ) | ||
| request_context = _request_context_with_messages( | ||
| [ModelRequest(parts=[UserPromptPart(content="!" * 200_288)])] | ||
| ) | ||
|
|
||
| with patch( | ||
| "conductor.providers._pydantic_ai.compaction._estimate_context_tokens", | ||
| new=AsyncMock(side_effect=RuntimeError("estimator exploded")), | ||
| ): | ||
| result = await capability.before_model_request(_make_run_context(), request_context) | ||
|
|
||
| inner.before_model_request.assert_called_once() | ||
| assert result is compacted | ||
|
|
||
|
|
||
| class TestTierFallback: | ||
| """Requirement: a failing non-final tier still yields to the final tier.""" | ||
|
|
||
|
|
@@ -920,9 +974,9 @@ def callback(event_type: str, data: dict[str, Any]) -> None: | |
| assert any(e[0] == "agent_compaction_complete" for e in events) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_gate_measurement_failure_skips_without_latch_or_event(self) -> None: | ||
| # A failing gate estimate warns, returns the | ||
| # original context, emits no event, and does not engage the latch. | ||
| async def test_gate_measurement_failure_uses_fallback_without_latch(self) -> None: | ||
| # Requirement: a failed primary estimate uses the conservative fallback | ||
| # without disabling compaction for later requests. | ||
| events: list[tuple[str, dict[str, Any]]] = [] | ||
|
|
||
| def callback(event_type: str, data: dict[str, Any]) -> None: | ||
|
|
@@ -942,7 +996,10 @@ def callback(event_type: str, data: dict[str, Any]) -> None: | |
|
|
||
| assert result is request_context | ||
| assert capability._disabled is False # type: ignore[attr-defined] | ||
| assert not events | ||
| assert [event_type for event_type, _ in events] == [ | ||
| "agent_compaction_start", | ||
| "agent_compaction_complete", | ||
| ] | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_telemetry_failure_keeps_compacted_result_without_latch(self) -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RECOMMENDED
Both new helpers claim token units for values that are bytes or mixed, which is the proximate cause of the unit mixing.
_count_utf8_bytesis documented as "Return a one-byte-per-token safety bound for token-dense text" — it returns a byte count. Whether that bounds tokens is the caller's assumption, not this function's contract, and describing a primitive by its caller's role is the comment most likely to rot._estimate_conservative_context_tokensis worse: the name ends in_tokensand the return type is a bareint, but the value is neither tokens nor bytes. When a usage anchor is present, the harness returns provider tokens for the anchored prefix plus UTF-8 bytes for everything after — a mixed-unit quantity valid only as a one-sided upper bound. Nothing anywhere says "do not treat this as tokens," and three lines later it's assigned straight intobefore_estimateand shipped astokens_before.Also worth flagging:
_estimate_context_tokensisasync defwhile its new sibling is a plaindef, though both wrap the same synchronous harness call. Given the project's "async/await for all provider operations" convention, the asymmetry reads as meaningful when it isn't.Suggested fix: rename to
_utf8_byte_lengthand_estimate_context_utf8_bytes, and state the unit plainly:and for the estimator, document that with an anchor it returns
anchor tokens + post-anchor bytes, so it's comparable only against the raw window and must not be reported as a token count. Under those names,before_estimate = max(before_estimate, byte_bound)reads immediately as the bug it is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in e8490bf, with names that state the mechanism now that the byte count is gone entirely:
_density_text_token_bound(text)— the docstring states the result is in TOKENS, documents the three content classes (ordinary prose → chars/4, substantial non-ASCII share → chars, whitespace-poor ASCII → chars/2), and notes the residual multi-token-per-character emoji case as a known undercount rather than claiming a hard bound._estimate_context_tokens_density(messages, params)— the harness-anchored safety estimate; the docstring states it is comparable against the window and the target and must never be mixed into token telemetry._estimate_context_tokens_independent(messages)— the fallback, documented as sharing no code with the primary path.The async/sync asymmetry you flagged is removed as well: all three estimators are uniformly
async(the primary's docstring notes the underlying harness call is synchronous and the signature is convention).