feat(runtime): Phase C - real runtime telemetry - #73
Conversation
Implements Native Runtime v2.0 Phase C (docs/NATIVE_RUNTIME_V2_SPEC.md §2, Phase C) -- surfaces production-meaningful telemetry from real SessionManager/AdapterManager state instead of placeholders. - SessionManager: renamed HasPendingAdapter -> HasBoundAdapter and corrected the class doc / load message. SessionManager never touches adapter state (that's AdapterManager's job, coordinated by RuntimeOrchestrator) so it can only honestly report whether the binding specifies an adapter, not whether one was attached -- the old "pending AdapterManager support" wording was stale, AdapterManager ships and applies adapters. - AdapterManager: new GetResidencySnapshot() -- a read-only, non-blocking, per-role view of ActiveCount/ConversationsCreated/ ForceRecycle plus a computed AdapterRoleResidencyStatus (Healthy/RecyclePending/Degraded/AtHardLimit). _entries is now a ConcurrentDictionary (was Dictionary) so this synchronous telemetry read can never see a torn write from the async minting pipeline -- the async _gate still exclusively serializes the higher-level mutation flow, unchanged. - RuntimeOrchestrator: new GetResidencySnapshot() forwarding to AdapterManager; RuntimeReservationSnapshot gained RejectedAdmissionCount/LastRejectionReason, a lifetime tally of every admission denial (both the fail-closed "no budget configured" case and a real capacity denial), guarded by the existing _telemetryGate pattern. - IRoleRuntime: two new interface members with default implementations (null/empty) -- NativeRoleRuntime overrides both to forward to its orchestrator; other implementers (fakes, scripted test runtimes) silently inherit the honest default, no breakage. - EstimatedVramBytes is now a REAL measurement: NativeVramProbe grew TryQueryCurrentProcessVramBytes() (nvidia-smi per-process accounting, filtered to this process's PID), replacing the old base+adapter file-size guess. Factored the shared bounded-subprocess logic out of the Phase B live-budget query into one RunNvidiaSmi helper so the deadlock fix from that PR isn't duplicated in a second copy. - MainWindow: logs the initial VRAM admission snapshot via the existing Activity Log pattern once the native runtime is constructed (total/reserved/available) -- the first real UI-visible use of what Phases A/B/C track. Residency isn't logged there since it's always empty at construction time (no role has streamed yet). Caught and fixed a real regression before it shipped: an existing non-gated test (NativeRoleRuntime_SchedulerDenial_Returns_ ClearFailure_Before_ModelLoad) asserted EstimatedVramBytes was always non-null, based on the old file-size guess. With the real-measurement change, a test process that never actually loaded a model correctly reports null -- updated the assertion to match the honest new behavior instead of preserving the old dishonest one. Deferred, not attempted: the full real-model /verify Phase C describes (drive a native role, read residency mid-flight) needs a real GGUF via THEORC_TEST_GGUF, unavailable in this environment -- same honest boundary as the existing gated native-smoke tests. Verified instead: every new accessor's empty/baseline/denied-state behavior (real code paths, no model needed), and the live VRAM queries genuinely re-verified against this machine's real GPU via a throwaway scratch program (not committed). Verified: all 7 consumer projects build clean (0 warnings, 0 errors). Full OrchestratorIDE.UnitTests suite: 599 passed, 0 failed, 4 skipped (same pre-existing THEORC_TEST_GGUF-gated skips). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 52 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 (1)
📝 WalkthroughWalkthroughThe runtime now exposes native VRAM and adapter residency telemetry, records admission denials in reservation snapshots, updates adapter-state reporting, logs VRAM admission details in the UI, and adjusts tests for the new telemetry behavior. ChangesRuntime telemetry and admission state
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NativeRoleRuntime
participant RuntimeOrchestrator
participant NativeVramProbe
participant AdapterManager
NativeRoleRuntime->>RuntimeOrchestrator: GetReservationSnapshot()
RuntimeOrchestrator-->>NativeRoleRuntime: Reservation and rejection telemetry
NativeRoleRuntime->>AdapterManager: GetResidencySnapshot()
AdapterManager-->>NativeRoleRuntime: Per-role residency entries
NativeRoleRuntime->>NativeVramProbe: TryQueryCurrentProcessVramBytes()
NativeVramProbe-->>NativeRoleRuntime: Current process VRAM bytes or null
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
OrchestratorIDE/Core/Runtime/IRoleRuntime.cs (1)
458-475: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProbe VRAM before acquiring
_telemetryGate.Line 475 can hold the telemetry lock for the probe’s three-second timeout, blocking
GetHealth,GetStats, and other failure updates. Capture the measurement before entering the lock.Proposed fix
private void RecordFailure(RuntimeRole role, RuntimeRoleBinding binding, string? runtimeMessage) { + var estimatedVramBytes = NativeVramProbe.TryQueryCurrentProcessVramBytes(); + lock (_telemetryGate) { ... _lastStatsByRole[role] = new RuntimeStats( RuntimeName, ActiveModel: FormatActiveModel(binding), - EstimatedVramBytes: NativeVramProbe.TryQueryCurrentProcessVramBytes()); + EstimatedVramBytes: estimatedVramBytes);🤖 Prompt for 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. In `@OrchestratorIDE/Core/Runtime/IRoleRuntime.cs` around lines 458 - 475, Update RecordFailure to call NativeVramProbe.TryQueryCurrentProcessVramBytes before acquiring _telemetryGate, store the result locally, and use that captured value when constructing RuntimeStats inside the lock. Preserve the existing health and stats updates.OrchestratorIDE/Core/Runtime/RuntimeOrchestrator.cs (1)
365-366: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose fail-closed rejections instead of discarding their snapshot.
Line 266 records missing scheduler/budget as a rejection, but these lines make that count and reason permanently inaccessible. Return a zero-budget snapshot when admission fails closed; reserve
nullfor the explicit unbudgeted opt-out.Proposed fix
- if (_scheduler is null || _budgetProvider is null) - return null; + if (_scheduler is null || _budgetProvider is null) + { + if (_allowUnbudgetedExecution) + return null; + + lock (_telemetryGate) + return new RuntimeReservationSnapshot( + [], + TotalBytes: 0, + ReservedBytes: 0, + AvailableBytes: 0, + RejectedAdmissionCount: _rejectedAdmissionCount, + LastRejectionReason: _lastRejectionReason); + }🤖 Prompt for 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. In `@OrchestratorIDE/Core/Runtime/RuntimeOrchestrator.cs` around lines 365 - 366, Update the fail-closed guard for _scheduler and _budgetProvider so admission rejection returns a zero-budget snapshot rather than null, preserving the recorded rejection count and reason. Keep null reserved exclusively for the explicit unbudgeted opt-out path.
🤖 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/AdapterManager.cs`:
- Around line 352-358: Update the AdapterRoleResidency projection to capture the
relevant counter values from kv.Value once into locals, then populate
ActiveCount and ConversationsCreated from those captured values. Derive Status
using the same captured counters rather than calling ComputeResidencyStatus with
kv.Value, including the equivalent projection at the additional occurrence.
---
Outside diff comments:
In `@OrchestratorIDE/Core/Runtime/IRoleRuntime.cs`:
- Around line 458-475: Update RecordFailure to call
NativeVramProbe.TryQueryCurrentProcessVramBytes before acquiring _telemetryGate,
store the result locally, and use that captured value when constructing
RuntimeStats inside the lock. Preserve the existing health and stats updates.
In `@OrchestratorIDE/Core/Runtime/RuntimeOrchestrator.cs`:
- Around line 365-366: Update the fail-closed guard for _scheduler and
_budgetProvider so admission rejection returns a zero-budget snapshot rather
than null, preserving the recorded rejection count and reason. Keep null
reserved exclusively for the explicit unbudgeted opt-out path.
🪄 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: 8fd773e2-e952-417f-82cf-3453016cc5b3
📒 Files selected for processing (10)
OrchestratorIDE.Avalonia/MainWindow.axaml.csOrchestratorIDE.UnitTests/AdapterManagerTests.csOrchestratorIDE.UnitTests/NativeRuntimeTestSupportTests.csOrchestratorIDE.UnitTests/RuntimeOrchestratorTests.csOrchestratorIDE.UnitTests/SessionManagerTests.csOrchestratorIDE/Core/Runtime/AdapterManager.csOrchestratorIDE/Core/Runtime/IRoleRuntime.csOrchestratorIDE/Core/Runtime/NativeVramProbe.csOrchestratorIDE/Core/Runtime/RuntimeOrchestrator.csOrchestratorIDE/Core/Runtime/SessionManager.cs
GetResidencySnapshot's projection read ActiveCount/ConversationsCreated as two separate Volatile.Read calls, then ComputeResidencyStatus re-read ConversationsCreated/ForceRecycle a third/fourth time. A concurrent mint between those reads could produce a displayed count and status describing different instants (e.g. ConversationsCreated=23 shown alongside Status=RecyclePending, which needs >= 24). Fixed by capturing each volatile value once into a local, then deriving both the record fields and the status from those same captured values -- ComputeResidencyStatus now takes the captured ints/bool directly instead of a RoleEntry it could re-read from. Verified: build clean, full test suite 599 passed / 0 failed / 4 skipped (same pre-existing gated skips). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: sync ROADMAP/CURRENT_STATE with Native Runtime Phases A-C Phases A (fail-closed admission boundary), B (live VRAM budget), and C (real telemetry) of docs/NATIVE_RUNTIME_V2_SPEC.md merged (#70, #72, #73) since these two files were last touched. Both had gone stale: - ROADMAP.md's Phase 3/4 rows described exactly the "remaining" work those three PRs closed (OrcScheduler wired into AdapterManager, telemetry surfaced) as still open. Corrected, and added a pointer to the new spec as the current foundation-hardening plan alongside the existing RUNTIME_PHASE0_SPEC.md contracts link. - CURRENT_STATE.yaml's native_runtime note predated all three phases. Added an accurate summary of what's landed, explicit that this is foundation hardening, not a default-runtime change (Phase D and the default-runtime flip remain open, per the spec's own scope). Docs-only, no code touched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: address grok review findings on PR #74 Four real accuracy gaps caught by an external review (Tools/grok-review.ps1 -Mode full): 1. CURRENT_STATE.yaml overclaimed Phase B as fully landed -- only the live VRAM budget READ shipped; the cost ESTIMATE side (OrcScheduler.EstimateRequiredBytes, still GGUF-file-size-only) was deliberately deferred in that same PR (#72). Corrected to name the gap explicitly. 2. ROADMAP.md's Phase 3 "Remaining" bullet had the same gap -- only named Phase D as open, omitted the deferred estimate work. 3. ROADMAP.md's "Last updated" banner (2026-07-17) was inconsistent with the 2026-07-19 status this PR stamps elsewhere in the same document. Updated the banner and clarified the doc's update policy allows incremental updates between releases, not just at release time. 4. NATIVE_RUNTIME_V2_SPEC.md's own banner still said "no implementation lands with this document" with no landed-phase status -- true in the narrow sense (implementation lands via separate PRs, exactly as designed) but misleading to a reader who'd reasonably read it as "nothing implemented yet." Added an explicit, dated implementation- status line naming which phases have landed (A/B-read-side/C) and which remain open (D, Phase B's deferred estimate half). Re-validated: YAML still parses, all 55 markdown anchor links (4 new) resolve correctly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
What this is
Implementation PR for Phase C of
docs/NATIVE_RUNTIME_V2_SPEC.md§2/Phase C — surfaces production-meaningful telemetry from realSessionManager/AdapterManagerstate instead of placeholders. Follows Phase A (#70) and Phase B (#72), both merged.Will get an independent review (CodeRabbit) before merge, per the standing rule for Native Runtime PRs.
What ships
SessionManager: renamedHasPendingAdapter→HasBoundAdapter, corrected the class doc and load message. SessionManager never touches adapter state (that'sAdapterManager's job) — the old "pending AdapterManager support" wording was stale;AdapterManagerhas shipped and applied adapters since Phase 3.AdapterManager.GetResidencySnapshot()— new, read-only, non-blocking, per-role view ofActiveCount/ConversationsCreated/ForceRecycleplus a computedAdapterRoleResidencyStatus(Healthy/RecyclePending/Degraded/AtHardLimit)._entriesis now aConcurrentDictionary(wasDictionary) so this synchronous telemetry read can never see a torn write from the async minting pipeline — the async_gatestill exclusively serializes the higher-level mutation flow, unchanged.RuntimeOrchestrator: newGetResidencySnapshot()forwarding toAdapterManager;RuntimeReservationSnapshotgainedRejectedAdmissionCount/LastRejectionReason— a lifetime tally of every admission denial, guarded by the existing_telemetryGatepattern.IRoleRuntime: two new interface members with default implementations (null/[]) —NativeRoleRuntimeoverrides both to forward to its orchestrator; other implementers (fakes, scripted test runtimes used across several other test files) silently inherit the honest default, no breakage.EstimatedVramBytesis now a real measurement:NativeVramProbegrewTryQueryCurrentProcessVramBytes()(nvidia-smi per-process accounting, filtered to this process's PID), replacing the old base+adapter file-size guess. Factored the shared bounded-subprocess logic out of Phase B's live-budget query into oneRunNvidiaSmihelper so the deadlock fix CodeRabbit caught on feat(runtime): Phase B - live VRAM budget for native admission #72 isn't duplicated in a second copy.MainWindow: logs the initial VRAM admission snapshot via the existing Activity Log pattern once the native runtime is constructed — the first real UI-visible use of what Phases A/B/C track. Residency isn't logged there since it's always empty at construction time (no role has streamed yet); there's no natural hook point for a mid-flight log without touching the chat-streaming call sites, which is out of scope for a read-only accessor phase.A real regression caught before it shipped
An existing, non-gated test (
NativeRoleRuntime_SchedulerDenial_Returns_ClearFailure_Before_ModelLoad) assertedEstimatedVramByteswas always non-null, based on the old file-size guess. With the real-measurement change, a test process that never actually loaded a model correctly reportsnull— I updated the assertion to match the new, honest behavior rather than preserving the old dishonest one. Caught by re-running the full suite before committing, not by review — flagging it here so it's visible in review too.What's deliberately deferred (and why)
Phase C's own
/verifydescription ("drive a native role; read the reservation + residency snapshot mid-flight... and after disposal") needs a real GGUF viaTHEORC_TEST_GGUF, which is unavailable in this environment — same honest boundary the existing gated native-smoke tests already draw. What I verified instead:RejectedAdmissionCount: 1and confirms no phantom reservation was committed).TryQueryLiveNvidiaBudget()and the newTryQueryCurrentProcessVramBytes()return correct real values.Verification
OrchestratorIDE.UnitTestssuite: 599 passed, 0 failed, 4 skipped (same pre-existingTHEORC_TEST_GGUF-gated skips).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes