bug(optimization): KV trim_to_length retains the oldest window instead of the newest (#13030)

Problem

LayerKVCache.trim_to_length() keeps the oldest positions and discards the newest — the
opposite of what its own docstring says it is for.

kv_cache.py:254-278:

def trim_to_length(self, max_len: int) -> None:
    """Trim all layer caches to at most max_len filled positions.

    Useful for sliding-window attention or when sequence length exceeds
    a target budget. ...
    """
    ...
    if entry.filled_len > max_len:
        entry.filled_len = max_len

Storage is linear and append-only: update() writes new positions at [start:end] where
start = entry.filled_len (:374-376),
and get() returns entry.k[:, :filled_len, :, :]
(:193). So position 0 is the first
token and filled_len - 1 is the most recent. Setting filled_len = max_len therefore retains
[0, max_len) — the oldest tokens — and silently drops everything recent.

Sliding-window attention requires the most recent max_len positions. Every caller gets a
context window anchored at the start of the sequence that stops advancing.

Compounding: _write_to_entry raises ValueError on overflow
(:369-373) rather than evicting.
Combined with a trim that does not free the right positions, long generation either silently
corrupts context or hard-fails at max_seq_len.

Impact

High, and silent — wrong output rather than an error. No caller can implement a working sliding
window with this API.

Fix

Step 1 (correctness, trivial). Retain the newest max_len positions: shift the tail of k/v
down to offset 0 before setting filled_len, or track a base offset. Add a test asserting that after
update()-ing N positions and trim_to_length(W), get() returns the last W, verified by
tensor content and not just by shape.

Step 2 (efficiency, follow-up). Replace linear storage with a fixed-capacity circular ring for
sliding-window layers, so trimming is a pointer move with no copy, and keep linear append-only storage
only for full-attention layers. Note the reference sizing pattern: a ring slightly larger than the
window (e.g. window + one chunk of spare rows) absorbs chunked-prefill writes without a wrap mid-write.
get() then needs to be wrap-aware — either returning two segments or rolling into scratch.

Step 1 must not wait on step 2.

Acceptance

trim_to_length(W) retains the most recent W positions, asserted by content.
Generation past max_seq_len evicts rather than raising.

Part of #13030.

fix(optimization): trim_to_length must retain the newest positions, n…

Step 1 landed — ea91bc88d on Dev_new_gui (PR #13607).

ea91bc88d fix(optimization): trim_to_length must retain the newest positions, not the oldest (#13033) (#13607)

trim_to_length now relocates the newest max_len positions to offset 0 before moving filled_len, so a sliding window gets the tail of the sequence instead of the head.

Worth recording: the existing test was asserting the bug. test_trim_preserves_content_up_to_limit checked torch.allclose(r[0], k[:, :6, :, :]) — the oldest six. Implementation and test were wrong in the same direction, so the suite confirmed the defect rather than catching it. It now asserts the newest window and explicitly asserts the head does not match, so a symmetric fixture cannot pass by accident.

Also covered: the copy overlaps itself whenever max_len > filled_len / 2 (at seq=10, max_len=7 the ranges [3:10] and [0:7] share four positions), so the source is cloned first — copying a tensor onto an overlapping view of itself is undefined.

Evidence: all three new tests fail against the reverted source (3 failed, 4 passed) and pass with the fix — 53 passed for the file, 341 passed for the package, run under an interpreter with real torch 2.13.0+cpu because these tests skip silently otherwise.

Leaving this open for Step 2 — the circular-ring storage so trimming is a pointer move with no copy. The _write_to_entry overflow behaviour (raises rather than evicting) is also still open.

Sibling #13031 is PR #13608.

Closed — verified delivered and present in Dev_new_gui.

Evidence

Artifact	Value
PR	#13607 (merged)
Verified against	`git show origin/Dev_new_gui:<path>` on current base, not the PR diff

Verified in base — kv_cache.py:254 trim_to_length() now calls _retain_tail(entry, max_len) (line 281, helper at 291), and the docstring says "most recent positions". The window retained is the newest, not the oldest.

This issue was fixed but left open because merges into Dev_new_gui do not auto-close — Closes #N only fires on the default branch. Closed as part of a sweep that cross-referenced merged PRs against open issues and verified each against current base. Every acceptance criterion was checked; partial delivery was left open rather than closed.

Reopened by the batch-2c closure audit, verified against current origin/main. This closed on 2026-08-16 against the old base branch. D4 is fixed: kv_cache.py:281 keeps the newest positions, tested by test_trim_retains_the_newest_positions_not_the_oldest. AC2 (evict rather than raise) is not met: optimization/kv_cache.py:399 still raises KV cache overflow, and test_overflow_raises_value_error pins that behaviour.

refactor(optimization): one lazy-import primitive, and the accelerate…

refactor(optimization): collapse the forked accelerate loader (#13049 …

D4 is fixed in merged code — this is closable; the residual is now #17801

Verified against the PR base, not a working tree:

$ git show origin/main:autobot-backend/llm_shared/optimization/kv_cache.py | sed -n '291,305p'
    @staticmethod
    def _retain_tail(entry: "_LayerEntry", max_len: int) -> None:
        """Move the newest ``max_len`` positions of *entry* down to offset 0.
        ...
        start = entry.filled_len - max_len
        if max_len > 0:
            entry.k[:, :max_len, :, :].copy_(entry.k[:, start : entry.filled_len, :, :].clone())
            entry.v[:, :max_len, :, :].copy_(entry.v[:, start : entry.filled_len, :, :].clone())
        entry.filled_len = max_len

trim_to_length now calls _retain_tail, which moves the newest max_len positions down to
offset 0 — the opposite of the entry.filled_len = max_len this issue was filed against. The
docstring records the correction and why (#1964, #13033). #13030's task tree still shows this
child unticked.

Not closed by this comment — an AC tick needs an owner's call, and the fix's test coverage
should be confirmed against the ACs here first.

Two things this issue did not cover, now split out as #17801 (child of #13030,
blocked_by #13031):

grep -rn "trim_to_length" returns tests only — the window has never had a production caller.
_retain_tail is tail-only. The first caller will be a decode loop whose sequence starts
with a system prompt and task prefix; retaining the tail evicts exactly those, silently — the
same failure shape this issue was filed for, one level up.

Source: docs/research/long-horizon-document-parsing-ring-cache.md

Half delivered — AC1 is on `main`, AC2 is live. Do not close this as superseded.

Flagging because a duplicate sweep proposed closing this as superseded by #17801, and that would have destroyed a live criterion. Verified against main @ 8bd2d70b04.

AC1 — trim_to_length(W) retains the most recent W positions — DELIVERED. llm_shared/optimization/kv_cache.py:291 _retain_tail, called from :281, with kv_cache_test.py::test_trim_retains_the_newest_positions_not_the_oldest asserting it by content rather than by length. The docstring records the lineage: "Issue #1964; corrected in #13033."

AC2 — "generation past max_seq_len evicts rather than raising" — NOT delivered. _write_to_entry at :384 still refuses:

start = entry.filled_len
end = start + new_seq
if end > self._config.max_seq_len:
    raise ValueError(f"KV cache overflow: current={start}, adding={new_seq}, max_seq_len=...")

Its own docstring still cites the older behaviour — "Issue #1964: Raises ValueError if the write would overflow max_seq_len" — so the file records the pre-#13033 contract at the exact site #13033 was meant to change.

Why this matters beyond bookkeeping. This issue framed the two halves as compounding: long generation either silently corrupts context (AC1) or hard-fails at the ceiling (AC2). The corrupting half was fixed. The hard-fail half was not, and it is the one a user meets — a long conversation now raises instead of evicting its oldest positions.

#17801 does not carry AC2. It addresses the sliding window having no caller and quotes #13033's AC1 fix as already landed. Closing this into it would leave AC2 owned by nobody.

Disposition: keep this open for AC2 alone. AC1 can be ticked on the evidence above.

A method note, since it nearly went the other way: my first grep for the overflow raise returned nothing and I almost recorded AC2 as delivered. The pattern was right; head -8 truncated the match list before reaching line 398. A zero from a truncated search is not an absence.

Refs #17801

Navigation Menu

Repository navigation

bug(optimization): KV trim_to_length retains the oldest window instead of the newest (#13030) #13033

Problem

Impact

Fix

Acceptance

D4 is fixed in merged code — this is closable; the residual is now #17801

Half delivered — AC1 is on `main`, AC2 is live. Do not close this as superseded.

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

bug(optimization): KV trim_to_length retains the oldest window instead of the newest (#13030) #13033

Description

Problem

Impact

Fix

Acceptance

Activity

mrveiss commented on Aug 5, 2026

mrveiss commented on Aug 16, 2026

mrveiss commented on Sep 13, 2026

mrveiss commented on Sep 29, 2026

D4 is fixed in merged code — this is closable; the residual is now #17801

mrveiss commented on Oct 4, 2026

Half delivered — AC1 is on main, AC2 is live. Do not close this as superseded.

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Half delivered — AC1 is on `main`, AC2 is live. Do not close this as superseded.