fix(runtime): recycle a role's executor after any failed generation, not just NoKvSlot - #79
Merged
Conversation
…not just NoKvSlot
Native Runtime v2.0 Phase D cancellation leg (docs/NATIVE_RUNTIME_V2_SPEC.md):
added a real-GGUF-gated test driving cancellation mid-generation through a real
NativeRoleRuntime. It found a genuine bug: cancelling between a conversation's
Prompt(token)/InferUntilReadyAsync pair left the role's persistent BatchedExecutor
unable to serve ANY subsequent request -- InferUntilReadyAsync never observed
NoKvSlot (ruled out via THEORC_KVCACHE_DIAGNOSTICS, zero retries logged) but also
never saw RequiresInference clear within 1024 decode passes. A control test
confirmed two uncancelled calls in a row never hit this, isolating the corruption
to cancellation specifically.
This is the same root cause AdapterManager's existing ForceRecycle/MarkForRecycle
mechanism already documents for NoKvSlot ("disposed conversations not fully
returning their cells before the next one claims them"), just reached via a
cancelled decode instead of a completed one. Rather than chase the exact
LLamaSharp/native mechanics of why a cancelled Infer(ct) can leave cells stuck,
this reuses the existing, already-proven mitigation: NativeRoleRuntime's
StreamRoleCompletionAsync now calls MarkRoleDegraded(role) in its existing
catch-and-rethrow block, so ANY exception escaping generation -- cancellation or
otherwise -- forces the next mint on that role to build a fresh executor instead
of risking reuse of one that didn't complete cleanly.
Verified on real hardware: the cancellation test passes reliably across repeated
runs with the fix, and the full 619-test suite (including every gated real-model
lane) stays green. Also documents the finding in the spec's Phase D section.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
Owner
Author
|
@coderabbitai rate limit |
Contributor
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
This was referenced Jul 20, 2026
hardcoreerik
added a commit
that referenced
this pull request
Jul 20, 2026
…ifact (#81) * feat(runtime): Phase D real-model E2E lane with retained evidence artifact Native Runtime v2.0 Phase D's remaining DoD item: "the lane runs green on a real reference box and emits a retained evidence artifact." The negative fail-closed test (#78) and the cancellation leg (#79) both landed already; this closes the happy-path piece. NativeRuntimeE2ELaneTests drives discovery (ModelDepot.Scan) through a REAL admission decision -- a real OrcScheduler against a real live-queried NativeVramProbe budget, deliberately not the allowUnbudgetedExecution opt-out every other gated lane in this workstream uses -- through adapter lifecycle, real inference, and telemetry reads at three stages (before/mid-flight/after), writing a retained JSON evidence artifact regardless of pass or fail. NativeE2ELaneEvidenceStore follows the same schema_version/timestamp_utc/ app_version convention as the existing NativeRuntimeFallbackEvidenceStore and NativeRuntimeComparisonReportStore, under .orc/native-e2e-lane/ -- a sibling store, not a fork of the pattern, since this is the successful-run case those two don't cover (the fallback store only persists non-success outcomes). Verified on real hardware, 3 repeated runs: 25.7 tok/s, 284ms TTFT, 2.5GB measured VRAM. The captured telemetry surfaced a genuinely interesting, correct distinction: residency (ActiveCount) drops to 0 once the conversation disposes, but the VRAM reservation deliberately stays live with the loaded model rather than releasing -- confirming those two lifecycles are intentionally decoupled, not a leak. Full 637-test suite (including every gated real-model lane) stays green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(runtime): address CodeRabbit findings on the E2E lane PR Three real, valid findings from the first genuine CodeRabbit pass under the new .coderabbit.yaml config: 1. No bound on the real streaming call -- a hardware stall would hang the test indefinitely instead of failing with retained evidence. Added a 2-minute CancellationTokenSource. 2. Evidence was written to %AppData% instead of the repo's .orc/native-e2e-lane/ convention every other evidence store in this workstream uses, because WriteAsync was never given a workspaceRoot. Added a FindRepoRoot() helper (walks up from the test binary looking for OrchestratorIDE.slnx) and passed it through. Verified: evidence now lands under the actual repo, confirmed on a real run. 3. The evidence filename used DateTime.Now while the record's own timestamp_utc field used UtcNow -- inconsistent, and could sort wrong across machine timezones. Now captures one DateTimeOffset.UtcNow and threads it through both. Also fixed a minor unit-spacing nitpick in the spec doc (284ms -> 284 ms, 2.5GB -> 2.5 GB). Skipped one suggestion: CodeRabbit's prompt asked the spec's Phase D banner to say the E2E lane "has landed" -- this PR isn't merged yet, so that would be the overclaiming this whole workstream has deliberately avoided. Left as "in flight (PR pending)," accurate as of this commit. Re-verified on real hardware: full 637-test suite green, E2E lane test passes with evidence now correctly retained in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(runtime): address ultrareview findings on the E2E lane PR Three real nits from the cloud multi-agent review, all verified against current code before fixing: 1. NativeE2ELaneRunResult dropped the exception TYPE on failure, only keeping ex.Message -- forking the sibling NativeRuntimeFallbackEvidenceStore pattern this store's own doc claims to follow (that one captures both ErrorType and ErrorMessage). A failed run's evidence couldn't distinguish an admission denial from a native load crash from a timeout. Added ErrorType, populated as ex.GetType().Name, threaded through to the JSON record. 2. Assert.Multiple unconditionally indexed snapshots[1]/[2] assuming 3 entries, but the mid-flight snapshot is only appended once a token is actually yielded -- a failure BEFORE the first token (an admission denial or native load failure, exactly the case this lane's real, non-opted-out admission is designed to exercise) leaves Count at 2, and the unconditional index would throw ArgumentOutOfRangeException, burying the real failure already surfaced via `success`/`errorMessage` under confusing noise. Guarded the count-dependent asserts behind an explicit count check. 3. Two doc-only contradictions in NATIVE_RUNTIME_V2_SPEC.md, both introduced across this PR and earlier commits: the Phase B addendum still said "PR #77, in flight" after #77 had actually merged; and this PR's own Phase D section said the E2E lane "landed" while the banner correctly said "in flight (PR pending)" -- fixed the stale #77 reference to "landed", and reworded the Phase D section to match the banner's honest "PR pending" framing rather than claim this PR landed before it's merged. Re-verified on real hardware: full 637-test suite green, E2E lane test passes with error_type now present (null on the successful run) in the retained evidence artifact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
hardcoreerik
added a commit
that referenced
this pull request
Jul 20, 2026
…endum + D (#82) * docs: sync ROADMAP/CURRENT_STATE with Native Runtime v2.0 Phase B addendum + D Both files still described Phase B's cost-estimate half as deliberately deferred and Phase D as untouched -- stale since PR #76 (cost estimate), #77 (load-time measurement), #78 (negative fail-closed test), and #79 (cancellation-corruption fix) all landed since the last sync. Updates the Native Runtime v2.0 direction table in ROADMAP.md and the native_runtime note in CURRENT_STATE.yaml to reflect current reality: Phase B fully landed (estimate + measurement), Phase D essentially complete pending the E2E evidence-lane PR (#81, open). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: mark E2E evidence lane landed (PR #81 merged), not pending CodeRabbit finding: ROADMAP.md's banner still said the E2E lane was "built, verified on real hardware, and pending merge" while CURRENT_STATE.yaml's parallel note omitted that qualifier entirely -- inconsistent, and now stale either way since PR #81 actually merged. Updated both to say landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
/verifyrequirement, and it caught a real bug in the process.BatchedExecutorunable to serve any subsequent request —InferUntilReadyAsyncnever hitNoKvSlot(confirmed viaTHEORC_KVCACHE_DIAGNOSTICS, zero retries logged) but also never sawRequiresInferenceclear within 1024 decode passes. A control test confirms two uncancelled calls in a row never hit this — isolating the corruption to cancellation specifically.AdapterManager's existingMarkForRecycle/ForceRecyclemechanism (already proven for the analogousNoKvSlot"disposed conversations not fully returning their cells" case) instead of inventing something new.NativeRoleRuntime.StreamRoleCompletionAsync's existing catch-and-rethrow block now also callsMarkRoleDegraded(role), so any exception escaping generation forces the next mint on that role to build a fresh executor.Infer(ct)call can leave cells stuck — that's a deeper native-interop question out of scope for this pass; the fix is a conservative, already-precedented mitigation, not a guess.Test plan
Cancellation_MidGeneration_PropagatesAndLeavesRoleReusable(real GGUF viaTHEORC_TEST_GGUF): reproduces the bug without the fix, passes reliably (3 repeated runs) with itTwoSequentialUncancelledCalls_OnSameRole_BothSucceed: confirms the bug is cancellation-specific, not a general reuse issuedotnet build OrchestratorIDE.slnxclean🤖 Generated with Claude Code