fix(engine): honor session max_seq_len; stop decode no-op past ctx 4096 - #59
Merged
Conversation
Sessions requesting a larger context window than the model-load default
(4096) silently degenerated: decode past position 4096 returned GEIST_OK
at ~0.54 ms/tok while the forward never ran.
Three stacked failures:
1. transformer_session_alloc ignored opts.max_seq_len — the state-level
RoPE cos/sin tables and per-session KV caches stayed sized to the
4096 default set at state-create time.
2. No bounds check anywhere — positions >= max_seq_len indexed past the
RoPE tables and memcpy'd past the KV buffers.
3. ops->prefill (void) and op_decode_step (-1 sentinel) swallowed the
failure, so the engine reported GEIST_OK with a garbage forward.
Fix:
- session_alloc now grows st->max_seq_len and rebuilds the shared RoPE
tables (grow-only; allocate_runtime_rope is re-runnable) when a
session requests a larger window, before sizing its KV caches.
Multi-session safe: bigger tables serve smaller windows; each session
records its own capacity in sess->max_seq_len.
- transformer_check_kv_room() guards all KV-appending paths (text
prefill, audio prefill, verify_forward, decode/audio-advance) ->
GEIST_E_TOO_MANY_TOKENS instead of out-of-bounds writes.
- Engine surfaces prefill kv_len shortfall and decode -1 as real errors.
- bench_session_throughput sizes its window from PP+TG and honors
GEIST_BENCH_BACKEND (was hardcoded cpu_neon).
Verified (cpu_neon + cpu_scalar): pp4096/pp6144 decode 64-71 ms/tok (was
0.54 no-op); 32/32 token parity neon<->scalar across the 4096 boundary;
oversized-prefill reject covered by test_session_lifecycle_int.
Known ceiling: KIVI mode's fixed 64 KB scores arena caps max_seq_len at
16384 (fails cleanly; marked with a ponytail comment). FP32/INT8 KV modes
are unaffected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…terjee-92f213 # Conflicts: # tests/bench_session_throughput.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A
geist_sessioncreated withopts.max_seq_lenlarger than the model-load default (4096) silently degenerated once context crossed position 4096:geist_session_decode_stepreturnedGEIST_OKat ~0.54 ms/tok while the forward pass never actually ran.Repro (before this change, on a Metal-enabled build):
Root cause
Three stacked failures:
transformer_session_allocreadm_max, kv-mode, and sampler opts but nevermax_seq_len. The state-level RoPE cos/sin tables and per-session KV caches stayed sized tost->max_seq_len, set from the model-load default (4096) at state-create time.GEIST_E_TOO_MANY_TOKENSwas in the enum but never raised. Positions>= max_seq_lenindexed past the RoPE tables andmemcpy'd past the KV buffers.ops->prefillreturns void andop_decode_stepreturns a-1sentinel the engine ignored, so the failure was swallowed and reported asGEIST_OK.Fix
arch_state.c): a session requesting a larger window bumpsst->max_seq_lenand rebuilds the shared RoPE tables (grow-only;allocate_runtime_ropeis now re-runnable) before sizing its KV caches. Multi-session safe — bigger tables serve smaller windows, and each session records its own capacity in the newsess->max_seq_len.forward.h):transformer_check_kv_room()guards all four KV-appending paths (text prefill, audio prefill,verify_forward, decode/audio-advance) →GEIST_E_TOO_MANY_TOKENSinstead of out-of-bounds writes.session.c): prefill failure detected via kv_len shortfall; decode-1now returns the backend error instead ofGEIST_OK.bench_session_throughput.c): sizes its window from PP+TG and honorsGEIST_BENCH_BACKEND(was hardcodedcpu_neon).Verification (cpu_neon + cpu_scalar — this is engine-level, not Metal-specific)
test_session_lifecycle_int.make format-checkclean;make test-unitgreen (22 passed, 0 failed); existingtest_multi_session_int/test_known_answer_e2e/test_pin_prefix_intgreen.Known ceiling
KIVI KV mode's fixed 64 KB scores arena caps
max_seq_lenat 16384 (fails cleanly; marked with aponytail:comment and upgrade path). FP32/INT8 KV modes are unaffected.🤖 Generated with Claude Code