Fix NoKvSlot/OOM crash on recurrent-architecture models (Qwen3.5) - #56
Merged
Conversation
ModelParams.SeqMax defaulted to 1, which plain-transformer llama.cpp paths tolerated but recurrent/hybrid architectures (Qwen3.5's Gated Delta Net layers) validate strictly -- every second-ever conversation on a role's executor failed find_slot/init_batch outright, 100% reproducible. Wiring SeqMax to AdapterManager.SequenceHardLimit fixes that, but the old hard limit (240, calibrated when native n_seq_max was effectively unlimited) turned into a live ~12GB rs-cache VRAM reservation for hybrid models and OOM-crashed the process instead. Lowered the limit to 40, keeping the reservation affordable (~2GB) while preserving recycle-threshold headroom. Verified live against Qwen3.5-9B-Q8_0 on a 16GB GPU: zero NoKvSlot errors, zero OOM crashes, full CF-7 smoke run completes in the expected timeframe (was previously either recycle-storming on every call or crashing native with 0xC0000005).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe runtime sequence hard limit changes from 240 to 40. Persistent model loading now sets ChangesSequence limit alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LLamaSharpRuntime
participant ModelParams
participant StatelessExecutor
LLamaSharpRuntime->>ModelParams: Configure persistent SeqMax
LLamaSharpRuntime->>ModelParams: Create stateless native-default parameters
LLamaSharpRuntime->>StatelessExecutor: Start streaming with stateless parameters
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OrchestratorIDE/Core/Runtime/LLamaSharpRuntime.cs`:
- Around line 231-240: Keep SeqMax at AdapterManager.SequenceHardLimit for the
persistent executor configuration, but update StreamCompletionAsync when
constructing StatelessExecutor to use a separate model-parameter instance with
SeqMax set to 1. Do not mutate the shared _modelParams instance; preserve its
larger sequence limit for conversation-backed execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2e070d6-9e57-49e6-8acb-1d97c687d3dd
📒 Files selected for processing (2)
OrchestratorIDE/Core/Runtime/AdapterManager.csOrchestratorIDE/Core/Runtime/LLamaSharpRuntime.cs
…0 reference Grok review (PR #56) flagged docs/CONTEXT_FABRIC_TEST_HARNESS.md still citing the old SequenceHardLimit=240 value without noting the 2026-07-14 change. Added §7b documenting the recurrent/hybrid-architecture failure mode and fix.
StreamCompletionAsync's StatelessExecutor reused _modelParams, which now carries SeqMax=SequenceHardLimit(40) for the persistent AdapterManager executors. StatelessExecutor only ever runs one sequence per call, so reusing those params made every stateless call reserve the same ~2GB hybrid-architecture rs-cache budget for no benefit, risking OOM under concurrent calls. Split into a separate _statelessModelParams instance with the native default SeqMax=1.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
4 tasks
hardcoreerik
added a commit
that referenced
this pull request
Jul 15, 2026
Escape #56 (MD018, avoids ATX-heading misparse) and add text language identifiers to two fenced code blocks (MD040).
hardcoreerik
added a commit
that referenced
this pull request
Jul 15, 2026
…nfra notes (#57) * docs: split CONTEXT_FABRIC_TEST_HARNESS.md into grading spec, bug history, infra notes The single 504-line doc mixed a normative grading specification with a chronological bug-investigation log and fleet/environment notes, making it hard for a reviewer to answer "how does this harness decide pass/fail" without wading through disproved hypotheses. Split per external review (Grok + DeepSeek combined critique): - CONTEXT_FABRIC_GRADING_SPEC.md: current-state normative spec, with a new decision flowchart, the full citation_precision algebra (including the aggregate-vs-per-question-mean distinction and the abstention edge case, neither previously documented), the held-out split's exact stratified 20%-per-category breakdown (pulled from the actual question-suite JSON, not transcribed), and B4's true provenance (a structurally different, smaller CF-6 benchmark, not a 120-question CF-7 run). - CONTEXT_FABRIC_BUG_HISTORY.md: the KV-cache investigation narrative and change history, moved verbatim with section numbers preserved (existing code comments cite §7/§7a directly). - CONTEXT_FABRIC_INFRASTRUCTURE_NOTES.md: fleet quirks plus a first-cut Known Model Compatibility table. Updated all 11 cross-references (6 code comments, 5 docs/config). Phase 1 of 4 (doc hygiene only, no code changes) addressing the combined Grok+DeepSeek review of the CF-7 test harness documentation. Gate metrics, exhaustive-heuristic hardening, and final compatibility-table cleanup are Phases 2-4. * Fix missed abbreviated doc reference (grok review) ContextFabricContracts.cs used the abbreviated form "CF_TEST_HARNESS.md", which the original 11-file cross-reference sweep missed (it searched for the exact full filename). Retargeted to CONTEXT_FABRIC_BUG_HISTORY.md §7, consistent with every other §7 reference. * Fix markdownlint nitpicks from CodeRabbit review Escape #56 (MD018, avoids ATX-heading misparse) and add text language identifiers to two fenced code blocks (MD040). * Fix decision flowchart double-counting totalCitations (grok review) The flowchart incremented totalCitations on both the H->L "regardless of outcome" edge and again inside the valid-citation node K, contradicting the code (ContextFabricValidation.cs:908,925) where totalCitations increments exactly once per non-null citation immediately after the null check, and validCitations increments separately only on the fully-valid path. Restructured so totalCitations++ happens on one edge (TC, gated only by the null check) and validCitations++ happens only at the end of the valid path (K), matching the source exactly. * Add missing null-claim and maxCitationsPerClaim error paths to flowchart (grok review) Flowchart omitted two real verifier paths (ContextFabricValidation.cs:887-891, 897-898): a null claim item (skips that claim entirely) and exceeding maxCitationsPerClaim (adds an error but does NOT skip citation processing, unlike the empty-claim-text check which I'd also mis-modeled as terminal). Re-verified the full 852-975 method line-by-line against this diagram before pushing. * Fix abstention-branch flowchart: independent checks, not sequential gates (grok review) Three inaccuracies (ContextFabricValidation.cs:935-957): 1. draftClaims.Count>0 (ERR11) is an independent, always-evaluated check under ExpectAbstention, not gated behind Abstained==true and the "does not establish" phrase passing. 2. The non-abstention branch's ExpectedTerm/ExpectedSegmentId checks always run regardless of the unexpected-abstention check's outcome -- not terminal on that error as previously drawn. 3. ExpectAbstention is evaluated once after the whole claims loop, not per-claim -- detached the node from the per-claim scope. Also fixed the final PASS/FAIL convergence to be an unconditional fan-in from every error node (matching the additive, non-short-circuiting nature already described in the surrounding prose) rather than a malformed multi-source conditional edge. * Fix top-level gates falsely modeled as short-circuiting + groundedTrace scope (grok review) Two more inaccuracies: 1. schemaVersion/answer-length/claim-count checks (ContextFabricValidation.cs:862-879) don't return/continue on failure -- they always fall through into claim processing, same pattern already correctly modeled for the per-claim checks. Gave them real forward edges and removed the now-duplicate direct-to-PASS edges for ERR/ERR2/ERR3/ERR4/ERR4c (they already flow forward, listing them in the final fan-in too created a contradictory two-destination edge). 2. groundedTrace (the ExpectedTerms match source) includes claim.Text, not just answer text and citation quotes (ContextFabricValidation.cs:948-950) -- updated both the flowchart node and §6.4 prose. * Fix stale BuildExhaustiveAnswer line number (grok review) Carried over ~740 from the original doc without verifying against current source; actual location is line 962. Also re-verified every other line number citation in this doc against current source in this pass. * Fix CF_TEST_RESULTS.md lead-in wording after doc split (grok review) The link target was correctly retargeted to CONTEXT_FABRIC_BUG_HISTORY.md in the Phase 1 commit, but the surrounding sentence still said "how the harness itself grades answers" -- that's Grading Spec content, not what Bug History §7/§7a (NoKvSlot investigation) actually covers. Split into two sentences pointing at the correct doc each. * Document Tier 1c hyphenated-identifier anchor match in BuildExhaustiveAnswer (grok review) Section 5.3 described only the entity-scoped/category-wide unigram classification, omitting the higher-precedence Tier 1c anchor-match path (ContextFabricFeasibilityRunner.cs:1006-1015, landed per CF_RETRIEVAL_IMPROVEMENT_PLAN.md §1c) that resolves hyphenated identifiers via verbatim substring match before the unigram heuristic ever runs. Also updated the "known limitation" note: Tier 1c already fixed the specific ledger-01-vs-ledger-09 collision originally cited as the motivating example; the residual heuristic risk now applies specifically to the non-hyphenated-identifier fallback path. * Clarify groundedTrace only includes valid citation quotes (grok review) §6.4 didn't specify that citation quotes feeding ExpectedTerm matching are only from citations that already passed NormalizeCitation -- normalizedClaims (the source of groundedTrace) is built from the `citations` list, which only gets appended to at ContextFabricValidation.cs:927, after segment-exists and quote-grounded checks pass. A hallucinated citation's quote text cannot be used to satisfy an ExpectedTerm. * Complete B4 gate check enumeration: 6 checks, not 4 (grok review) LoadHiveAcceptanceGate also fails closed on questions[].answerValidated and stitchCases[].validated (or either array being empty), not just passed/gateMode/readerNodeCount/verifiers -- ContextFabricBaselineRunner.cs:129-180. * Fix stale product-code-sharing claims and flowchart edge-label ambiguity (grok review) 1. Section 5.1 claimed BuildEvidencePack "is the same evidence selection used by FabricNativeReaderService and HiveNativeRoleExecutorAdapter in the real product" -- carried over from the original doc without verification. Neither file references BuildEvidencePack; the actual product path is FabricAskService via the structurally separate EvidencePackBuilder.cs. Corrected to describe them as conceptually similar but independent implementations. 2. Section 5.2 claimed B2 uses "the same IDF-weighted, budget-fill approach as B3" -- also stale. B2 (BuildTopKText) scores whole segments with the stricter 3-char Tokenize and a plain 1/documentFrequency weight; B3 (BuildEvidencePack) scores cards with TokenizeForScoring and ScoreTextIdf. Corrected with the actual algorithmic differences. 3. The flowchart's "all independent checks" edge label on the ERR9/ERR10/P fan-in into Q was ambiguous -- could be misread as claiming ERR10 fires independently of draft.Abstained, when the graph topology already correctly gates it behind O(Abstained)==yes. Reworded to only claim what the edge actually shows: Q runs regardless of which upstream path fired. * Rewrite BuildEvidencePack description: full Tier 1a/1b/1.5/2.5 pipeline, not plain IDF (grok review) Section 5.1 described only the original Tier-0 IDF+budget-fill algorithm from commit c68e01c. The current implementation (ContextFabricFeasibilityRunner.cs:661-830) has accreted anchor/proximity-pair extraction, lexicographic AnchorScore-then- TermScore ranking (not a single ScoreTextIdf call), coverage-aware greedy fill, MultiHop-specific budget reservation, and a reference-chase pass -- the full retrieval-quality work tracked in CF_RETRIEVAL_IMPROVEMENT_PLAN.md. Rewrote to describe the current 5-step pipeline accurately. Also fixed the flowchart's node S label to say "a VALID citation's quote", matching the already-corrected §6.4 prose (commit 86bbc74) that this edge had drifted out of sync with. * Fix ScoreTextIdf misattribution and Tier 1a/1.5 mislabeling (grok review) 1. Section 5.2 claimed B3's TermScore comes from ScoreTextIdf; it doesn't -- BuildEvidencePack inlines the same plain 1/documentFrequency formula B2 uses. ScoreTextIdf is exclusive to BuildExhaustiveAnswer. Corrected to note the formulas are actually the same; the real B2/B3 gap is B3's additional AnchorScore/coverage-fill/MultiHop-budget/reference-chase layers B2 entirely lacks. 2. Section 5.1 combined anchor extraction and proximity-pair extraction into one "Tier 1a" step; CF_RETRIEVAL_IMPROVEMENT_PLAN.md marks ExtractProximityPairs as the later Tier 1.5. Split into separate numbered steps with correct tier labels, renumbering the rest of the list. * Fix stale step-count cross-ref and imprecise capture-point pointer (grok review) 1. §5.2's "§5.1 steps 1-5" went stale when the prior commit renumbered §5.1 to 6 steps (Tier 1a/1.5 split). 2. fabric_v0.json's "capture point" comment pointed solely at CONTEXT_FABRIC_GRADING_SPEC.md, but "capture point" describes the whole run/capture infrastructure, not just the scoring rules -- added CONTEXT_FABRIC_BENCHMARK_MANIFEST.md (the run/capture recipe) alongside it. * Fix systems table B3 description to match §5.1's corrected product-path claim (grok review) §2's table still called B3 "the actual product answering path" after §5.1 was corrected to say BuildEvidencePack is a separate harness-only implementation from the shipped product's FabricAskService/EvidencePackBuilder path -- an internal contradiction between two sections of the same doc.
3 tasks
hardcoreerik
added a commit
that referenced
this pull request
Jul 15, 2026
…anually (#58) * Fix Qwen3.5 reader-stage failure: apply enable_thinking suppression manually CF-7 reader calls against Qwen3.5-9B were reporting 0/128 segment acceptance on every run despite the SeqMax runtime fix (PR #56). Root cause: Qwen3.5's own GGUF chat template implements the standard enable_thinking opt-out pattern -- its trailing add_generation_prompt block appends an empty, pre-closed <think></think> seed whenever enable_thinking isn't explicitly true, causing the model to skip straight to its real answer. LLamaSharp's LLamaTemplate (a minimal Jinja subset) renders through "<|im_start|>assistant\n" without error but never evaluates that trailing conditional, so the seed never gets applied and the model free-generates its trained default: full reasoning mode, consuming the reader's entire ~2000-token completion budget before any JSON is emitted. The previously-reported "16/120 and 1/120, both NO-GO" scores were not a capability measurement -- with zero segments ever ingested, B3 could only "pass" the 16 Unanswerable questions in the held-out set (correctly abstaining), meaning the score just reflected how many questions are Unanswerable, not model capability. ModelAdmissionGate had already flagged this exact risk when admitting Qwen3.5 as Provisional: "Visible reasoning traces can consume the response budget or precede the required JSON object." Fix: detect enable_thinking support generically from the raw tokenizer.chat_template metadata (not a filename/family check, so it covers future reasoning models using the same pattern) and append the same empty think-block the official template would render. Both new functions are pure/static with direct unit test coverage. Verified live (Qwen3.5-9B-Q8_0, 3-question smoke test): segment acceptance 0/128 -> 123/128, <think>-prefixed outputs 128/128 -> 0/128, mean completion tokens 2054 -> 294 (~7x faster per call, since none of the completion budget is wasted on reasoning). * Fix idempotency, exception-safety, and doc overclaim (grok review) 1. ApplyThinkingSuppression is now idempotent: no-ops if the prompt already contains a <think> marker, guarding against double-application and against a future LLamaSharp version that DOES evaluate the template's own seed. 2. Wrapped the thinking-suppression detection/application in its own try/catch inside ApplyEmbeddedTemplate. A failure there was previously uncaught, propagating to BuildPromptForLoadedModel's outer catch, which treats ANY exception from this method as "the embedded template doesn't work" and permanently falls back to ChatML for the rest of the session -- a much worse regression than just skipping suppression once. 3. Bug History §7c claimed PR #58 included "the first full-scale re-run" -- it doesn't yet; corrected to match Infrastructure Notes, which already said the re-score hadn't run. * Require both enable_thinking marker AND the literal seed text (grok review) SupportsThinkingSuppression previously matched on "enable_thinking" alone -- a template could reference that variable name for unrelated semantics or a differently-shaped seed, and blindly appending Qwen's exact <think>\n\n</think>\n\n text would be wrong for such a template, not just redundant. Now requires the literal seed text too, tying detection directly to the mechanism being replicated. Re-verified live against Qwen3.5-9B-Q8_0: identical results (123/128 segment acceptance, 0/128 think-prefixed, 294 mean completion tokens) confirming the tightened check still matches the real GGUF template. * Fix whole-prompt false-positive, document dual-gate, add missing coverage (grok review) 1. ApplyThinkingSuppression's idempotency guard scanned the WHOLE prompt for "<think>" -- any earlier message (conversation history, a document being analyzed) mentioning that literal text for unrelated reasons would false-positive and skip suppression entirely, re-enabling full reasoning mode for the whole call. Narrowed to check only the trimmed tail, which is the actual idempotency concern (double-application / template's own seed). 2. Bug History §7c described detection as matching only the "enable_thinking" marker; code (8cc8920) already requires both that AND the literal empty-seed text. Corrected the narrative. 3. Added regression tests: dual-gate rejects enable_thinking-present/ seed-absent templates, and the tail-only fix (item 1) doesn't false-positive on think-mentioning history content.
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
1 participant
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.
Summary
ModelParams.SeqMaxdefaulted to 1, which plain-transformer llama.cpp paths tolerated but recurrent/hybrid architectures (Qwen3.5's Gated Delta Net layers) validate strictly — the second-ever conversation on any role's executor failedfind_slot/init_batchoutright, 100% reproducible.SeqMaxtoAdapterManager.SequenceHardLimitfixes the NoKvSlot storm, but the existing hard limit (240, calibrated back when nativen_seq_maxwas effectively unlimited for transformer models) became a live ~12GB VRAM reservation for the recurrent-state ("rs cache") on hybrid models and OOM-crashed the native process instead (cudaMalloc failed, exit0xC0000005).SequenceRecycleThreshold(24).Test plan
dotnet test— 19/19 relevant unit tests pass (ModelDepot/AdapterManager/LLamaSharp filters)SequenceRecycleThreshold=24and recycles cleanly, matching designed behavior🤖 Generated with Claude Code
Summary by CodeRabbit