Skip to content

[Engine] evict redundant holders while preserving pinned prefixes - #2443

Open
KeyanHu-git wants to merge 11 commits into
Niko1221:mainfrom
KeyanHu-git:fix/2255-pinned-eviction
Open

KeyanHu-git wants to merge 11 commits into
Niko1221:mainfrom
KeyanHu-git:fix/2255-pinned-eviction

Conversation

@KeyanHu-git

@KeyanHu-git KeyanHu-git commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem and result

Pinning the same system prompt on every request fills the conversation cache with branches of one prefix. Upstream protects every branch, so later conversations cannot be saved even when the byte budget has room.

This change evicts redundant branches while keeping at least one holder of every pinned prefix. The same 15-request replay against v0.2.7, repeated on this ownership revision, gives:

Result v0.2.7 This PR This PR, async saves enabled
Parking refusals 8 0 0
Restored conversations 0 6 6
Input tokens processed on six revisits 1,778 90 90
Output sequences differing from v0.2.7 — 0 0

Related discussion

Fixes #2255. Related admission paths: #2253 and #2082.

Approach

flowchart LR
    A[Old branch A] --> P[Shared pinned prefix]
    B[Branch B] --> P
    C[Branch C] --> P
    A --> E[Evict A and save the new branch]
    B --> K[Prefix remains recoverable]
    C --> K
Loading

Evict unpinned entries first, then the oldest entry whose pinned prefixes all have another holder. Prefix identity includes token IDs, image identities and steering mode. A borrowed donor stays protected and counts as a surviving holder.

evictable_bytes() simulates the same removals, so admission and session-save reclamation agree with eviction about what can be released. Retained active K/V is accounted separately.

Snapshot replacement now preserves the same ownership order as destruction. SavedConversation and ConversationKv take the replacement by value and swap their members, leaving the complete old state to join its pending copies before releasing storage. This covers cache erasure, capture publication and file-load replacement through the types themselves; cache erasure needs no special handling. Moving transfers in-flight ownership. Copying still requires completed writes in the source, including its layers and stages.

Validation

Production revision: 3d502f47, based on v0.2.7 5e8a4cca. The current diff keeps the fix, regression tests and user documentation. Experimental records are preserved at the immutable revision below, outside the merge diff. Recorded requests, outputs, settings, source hashes and reproduction steps.

Passed:

  • Windows 11, Ryzen 5 7600X, RX 7900 XTX, Coder IQ1_M: 15 requests each for upstream, patched, and patched with STRATA_PARK_ASYNC=1 STRATA_CKPT_ASYNC=1. Three cache slots, 4 GiB cache budget, fixed expert placement, greedy generation and snapshot verification. All output IDs match; reuse results are above.
  • Apple Clang with -Wall -Wextra -Werror -fsanitize=address,undefined: 45,257 cache checks and 161 session-save checks. Tests cover shared/distinct pins, image and steering identity, donors, reclaimable-byte accounting, replacement, self-move, shifted neighbours and nested stage lifetime. File-format tests pass 198 checks, memory tests 31 checks, and stage split tests preserve allocation-failure state in 8/12/16 cases.
  • Full CUDA (sm_120), HIP (gfx1100), Metal and SYCL engine builds. CUDA ctest passes both cache and session-save checks.
  • HIP cache and session-save executables; git diff --check.

Failed: the original middle-erase lifetime counterexample report ASan heap-use-after-free; the patched path keeps all three buffers alive through their joins. A cache-test fixture had 21 tokens for a 22-token image and failed ASan; the missing reply token is now included. The earlier v0.2.7 refresh initially omitted a benchmark source from the transferred CUDA archive; restoring it fixed configuration. The current ownership revision passed all four builds.

Not run: CUDA, SYCL or Metal model execution. The async-config replay verifies outputs and reuse, not measured copy overlap.

Limits

Explicit constructors make the snapshot types non-aggregate; current repository consumers pass all four backend builds. Data-member layout and session-file format are unchanged. Copying a source with pending writes remains outside the copy contract.

The eviction search depends on the number of entries, checkpoints and prefix length. Logical freed bytes are used for accounting; physical-memory admission still resamples available memory. The model replay establishes cache reuse and matching outputs on this workload, with diagnostics enabled, rather than a throughput claim.

@qiuzan2001

Copy link
Copy Markdown

The eviction rule reads right to me (every pinned prefix keeps one holder, and each eviction re-checks). Two follow-ups, found by reading (C++ test not built here):

@KeyanHu-git

Copy link
Copy Markdown
Contributor Author

Thanks @qiuzan2001 for checking both interactions. The documentation fix is in 3e0780a.

You're right about evictable_bytes(). I reproduced the mismatch: with three holders of the same pin, the admission patches report zero evictable bytes, while this eviction rule can release two holders and retain the third.

The accounting needs to agree with repeated evict_oldest() calls. Simply summing every holder that is currently redundant would overcount: after earlier removals, the last holder must become non-evictable. I tested local integration prototypes for both #2253 and #2082 using the same pin-matching rule and tracking simulated removals without copying snapshot payloads. Both combined CPU test suites pass under ASan/UBSan, including 4,096 pin-membership combinations checking that the estimate matches actual eviction and preserves every pin. The same added tests fail with the original unpinned-only accounting. These are local CPU integration checks; the follow-up code still needs to be integrated and reviewed in whichever PR lands second. I also confirmed the textual conflicts and retained both sets of tests in the local combinations.

Since neither admission API is on main yet, I suggest landing this focused fix first, then updating the accounting in the admission PR that follows. If an admission PR lands first, #2443 will need that adjustment before merging.

@Niko1221, could you review the current fix and let us know whether that landing order works for you?

@KeyanHu-git

Copy link
Copy Markdown
Contributor Author

Follow-up: the cache-side accounting is now implemented in 613f78f. Keeping it in the cache defines the eviction contract in one place, while the admission policies remain in their respective PRs.

evictable_bytes() shares the pin-matching rule with eviction and tracks simulated removals, so it excludes retained K/V and the final holder of each pin. The query does not mutate the cache or copy snapshot payloads; up to 64 entries need no heap allocation. I also corrected the comment about memory release: logical evictable bytes do not guarantee an immediate RSS reduction.

The updated suite passes 44,581 checks on macOS, Windows and Linux, and full CUDA/HIP/SYCL builds pass. Local combinations with #2253 and #2082 pass 44,641 and 44,651 CPU checks under ASan/UBSan, including admission by evicting a redundant pinned holder. The earlier real-model comparison remains from 44877b3; this follow-up was validated through CPU tests and backend builds.

When rebasing #2253 or #2082, please retain this implementation instead of the unpinned-only definition, keep a single copy of the method, and preserve both sets of regression tests. The textual merge conflicts still need resolution. The PR description now records the implementation and validation details.

@Niko1221

Copy link
Copy Markdown
Owner

Thanks for the pull request.

We're resetting the tracker for 0.2.0-beta. There are more than 1,300 open issues and pull requests, too many for us to review properly, so we're closing everything that isn't being worked on right now. Closed here does not mean rejected.

To reopen it takes about two minutes:

  1. Rebase on main if it no longer applies, and re-run your checks on 0.2.0-beta.
  2. Start the title with its area: [Engine], [GUI], [TUI], [Benchmark], [Docs] or [Other].
  3. Fill in the pull request template: the problem and result, and the validation (what you ran, on which revision, what you saw).
  4. Comment /reopen. A check runs and either reopens the pull request or lists what is still missing.

The rules are in CONTRIBUTING.md.

@Niko1221 Niko1221 closed this Oct 19, 2026
@KeyanHu-git KeyanHu-git changed the title conversation cache: evict redundant holders without losing pinned prefixes [Engine] conversation cache: evict redundant holders without losing pinned prefixes Oct 20, 2026
@KeyanHu-git

Copy link
Copy Markdown
Contributor Author

/reopen

@github-actions github-actions Bot reopened this Oct 20, 2026
@github-actions

Copy link
Copy Markdown

Reopened, thank you. A maintainer will pick it up from here. If it conflicts with main, rebase it first; that speeds up review.

…fixes

Preserve one holder for every pinned prefix when evicting redundant parked
conversations. Match tokens, image identity and steering mode, and retain
the existing preference for unpinned victims.

Cover multiple independent pins, multi-pin entries, identity differences,
slot and byte pressure, and accounting.

With a borrow in progress, the guarded donor is never evicted but still
counts as a holder, and make_room's guard precheck uses the same
simulated eviction, so it neither refuses a borrow that redundant holders
can make room for nor evicts anything before refusing.

Fixes Niko1221#2255
Signed-off-by: Keyan Hu <phycheor@gmail.com>
Expose a non-mutating evictable_bytes estimate using the same pin-matching
rule as eviction. Track simulated removals so the last holder of each pin
is excluded, along with retained KV storage. Keep the normal cache-sized
scratch bitmap on the stack and avoid copying snapshot payloads.

Cover every assignment of three pins to four conversations, identity
separation, retained storage, and the stack/heap scratch boundary. Compare
the estimate against actual repeated eviction and verify pin survival.

Share the simulation with make_room's guard precheck, so a guarded
donor is excluded from the estimate while still holding its pins.

Signed-off-by: Keyan Hu <phycheor@gmail.com>
@KeyanHu-git
KeyanHu-git force-pushed the fix/2255-pinned-eviction branch from 613f78f to 64a2275 Compare October 20, 2026 09:38
@KeyanHu-git
KeyanHu-git marked this pull request as draft October 22, 2026 11:33
@KeyanHu-git KeyanHu-git changed the title [Engine] conversation cache: evict redundant holders without losing pinned prefixes [Engine] evict redundant holders while preserving pinned prefixes Oct 22, 2026
@KeyanHu-git
KeyanHu-git marked this pull request as ready for review October 22, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

conversation cache: once every slot holds a pinned prefix, nothing is parked any more ("skip parking ... exceeds available budget")

3 participants