Skip to content

fix grammar rejection cache: content-based keys, index-only values - #2434

Draft
Reithan wants to merge 1 commit into
LostRuins:concedo_experimentalfrom
Reithan:fix-grammar-memoization-bugs
Draft

fix grammar rejection cache: content-based keys, index-only values#2434
Reithan wants to merge 1 commit into
LostRuins:concedo_experimentalfrom
Reithan:fix-grammar-memoization-bugs

Conversation

@Reithan

@Reithan Reithan commented Sep 5, 2026

Copy link
Copy Markdown

Overview

Fix two bugs in the grammar rejection cache (candidates_memos) that prevented cross-step cache hits and introduced a use-after-free.

The cache stores results from reject_candidates_for_stack and persists across tokens for the duration of a request. Two problems prevented it from working correctly.

First, the hash function used pointer identity for the code_points field rather than the content at those pointers. Each step's apply call rebuilds the candidate list with new allocations, so the pointers differ across steps even when the content is identical. The cache could not produce hits across steps.

Second, cache values stored full candidate structs, which include raw code_points pointers into a buffer local to the apply call. Retaining those pointers after the call returns leaves dangling references; a cache hit in a later step dereferences freed memory.

The two bugs interacted: the hash bug prevented cross-step hits, which masked the use-after-free because the stale pointers were never read in practice.

Additional information

Changes:

  • Switch to a content-based hash: combine the candidate index, token ID, UTF-8 state, and the actual code-point sequence.
  • Store only rejection indices in the cache value, not full candidate structs.
  • On a cache hit, reconstruct the live candidate list by matching stored indices against the current step's data.

Existing grammar tests cover the rejection logic. The bugs required cross-step conditions that existing tests do not exercise, so no new tests are added.

Requirements

  • I have read and agree with the contributing guidelines.
  • AI usage disclosure: AI was used to identify the bugs and draft the fix; the commit was reviewed and approved by the author before push.

@LostRuins
LostRuins changed the base branch from concedo to concedo_experimental September 5, 2026 12:48
@LostRuins

Copy link
Copy Markdown
Owner

you should PR to the experimental branch, and i think this PR conflicts with your previous one. Also I don't actually think there is any performance improvement in fact I think that this actually performs worse.

@Reithan

Reithan commented Sep 6, 2026

Copy link
Copy Markdown
Author

Let me re-target to experiment and I'll mark this as draft and run a few more tests. 👍

@Reithan
Reithan marked this pull request as draft September 6, 2026 02:54
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.

2 participants