feat(runtime): Phase B - live VRAM budget for native admission - #72
Conversation
Implements the live-budget half of Native Runtime v2.0 Phase B (docs/NATIVE_RUNTIME_V2_SPEC.md Phase B). Ships the well-grounded half, defers the other. - New NativeVramProbe.TryQueryLiveNvidiaBudget(): a live nvidia-smi subprocess query (memory.total, memory.used), same pattern already proven safe in OrchestratorSetup's HardwareDetector.QueryNvidiaSmi, but for LIVE usage rather than a one-time install-detected total. Before this there was no "how much VRAM is free right now" query anywhere in the running app -- only a one-time, install-time TOTAL capacity probe in a different project/assembly. That gap is exactly why MainWindow/HiveService's budget providers hardcoded ReservedBytes: 0. - MainWindow and HiveService now pass the budget-building METHOD itself as budgetProvider (not a closed-over one-time snapshot), so RuntimeOrchestrator.EnsureAdmitted re-queries live VRAM on every admission, not just once at construction. Both fall back to the pre-Phase-B static-total behavior when the live probe is unavailable (non-NVIDIA GPU, nvidia-smi missing) -- never worse than before. - Widened Func<VramBudget>? to Func<VramBudget?>? in RuntimeOrchestrator/IRoleRuntime: the declared type didn't match reality (EnsureAdmitted already null-coalesces/throws on a null result, GetReservationSnapshot already null-checks it) -- this was producing a real CS8621 nullability warning once a genuinely nullable-returning method was passed directly. Deliberately deferred: a KV/rs-cache-aware cost-estimate addition to OrcScheduler.EstimateRequiredBytes. Investigated adding AdapterManager.SequenceHardLimit * ~50MB/slot as a fixed reservation (the number AdapterManager's own comments already document), but that overhead is specific to hybrid/recurrent-architecture models (Qwen3.5's Gated Delta Net layers) -- RuntimeModelAsset has no architecture metadata to distinguish those from plain-transformer models, so a flat always-on addition would over-reserve VRAM for the common case and risk denying legitimate admissions that used to fit. Shipping a half-baked generalization from one empirical data point was rejected in favor of shipping the well-grounded half now. Verified: all 7 consumer projects build clean (0 warnings, 0 errors). Full OrchestratorIDE.UnitTests suite: 596 passed, 0 failed, 4 skipped (same pre-existing THEORC_TEST_GGUF-gated skips). The live probe was verified for real against this machine's actual GPU (RTX 5070 Ti) via a throwaway scratch program (not committed) -- output matched a manual nvidia-smi reading taken independently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 37 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 (2)
📝 WalkthroughWalkthroughNative runtime budget providers now resolve nullable budgets per admission. A shared NVIDIA probe queries live VRAM with timeout and failure handling, while the daemon falls back to configured capacity. Tests cover probe results and repeatability. ChangesNative VRAM budget evaluation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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.Avalonia/MainWindow.axaml.cs`:
- Around line 2297-2304: Update TryBuildNativeHiveBudget to call
NativeVramProbe.TryQueryLiveNvidiaBudget() first and return its live budget when
available. If the probe does not produce a result, preserve the existing
fallback to the statically detected total, including its current ReservedBytes
behavior.
In `@OrchestratorIDE/Core/Runtime/NativeVramProbe.cs`:
- Around line 65-72: Update the process-reading flow around NativeVramProbe’s
StandardOutput handling so ReadToEnd is initiated asynchronously before
enforcing the QueryTimeout. Wait for process exit within the timeout, kill the
process and return null on timeout, then retrieve the completed output only
after successful exit without introducing an indefinite synchronous read.
🪄 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: fab9866c-0d57-43de-a778-33ee8523e128
📒 Files selected for processing (7)
OrchestratorIDE.Avalonia/MainWindow.axaml.csOrchestratorIDE.Daemon/HiveService.csOrchestratorIDE.NativeRuntime/OrchestratorIDE.NativeRuntime.csprojOrchestratorIDE.UnitTests/NativeVramProbeTests.csOrchestratorIDE/Core/Runtime/IRoleRuntime.csOrchestratorIDE/Core/Runtime/NativeVramProbe.csOrchestratorIDE/Core/Runtime/RuntimeOrchestrator.cs
Two real, CodeRabbit-caught bugs in the Phase B live-VRAM-budget PR: 1. MainWindow.TryBuildNativeHiveBudget() was never actually updated to call NativeVramProbe.TryQueryLiveNvidiaBudget() -- the comment on the call site claimed it did, but the method body was byte-identical to the pre-Phase-B code. Only the caller (budgetProvider wiring) was fixed; the method itself was not. HiveService's equivalent was correct -- this was isolated to MainWindow. Fixed by actually adding the live-probe call the comment already described. 2. NativeVramProbe.TryQueryLiveNvidiaBudget() called StandardOutput.ReadToEnd() before WaitForExit(timeout) -- ReadToEnd blocks until stdout closes, which only happens when the process exits, so a genuinely hung nvidia-smi would block indefinitely and the intended 3s timeout would never even be reached. Fixed with a CancellationTokenSource(QueryTimeout) bounding both the async read and the async exit under one shared deadline via Task.WaitAll, killing the process on cancellation. Verified: all 7 consumer projects build clean (0 warnings, 0 errors). Full test suite: 596 passed, 0 failed, 4 skipped (same pre-existing gated skips). Both fixes re-verified for real against this machine's GPU via a throwaway scratch program (not committed): 5 repeated calls through the fixed probe returned consistent correct values in 62-84ms each (not hitting the 3s timeout), confirming the deadlock fix didn't regress normal-case latency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both findings confirmed and fixed in 00081b0:
Both re-verified for real against this machine's GPU (not just unit tests): 5 repeated calls through the fixed probe returned consistent correct values in 62-84ms each, confirming the deadlock fix didn't regress normal-case latency. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 00081b0c
git diff 00081b0c^ 00081b0c -- \
OrchestratorIDE.Avalonia/MainWindow.axaml.cs \
OrchestratorIDE/Core/Runtime/NativeVramProbe.cs
ast-grep outline OrchestratorIDE.Avalonia/MainWindow.axaml.cs --items all --match 'TryBuildNativeHiveBudget|BuildExperimentalNativeRoleRuntime'
ast-grep outline OrchestratorIDE/Core/Runtime/NativeVramProbe.cs --items all --match 'NativeVramProbe|TryQueryLiveNvidiaBudget'
rg -n -C 5 '\bTryQueryLiveNvidiaBudget\s*\(' \
OrchestratorIDE.Avalonia/MainWindow.axaml.cs \
OrchestratorIDE.Daemon/HiveService.cs \
OrchestratorIDE/Core/Runtime/NativeVramProbe.cs \
OrchestratorIDE.UnitTests/NativeVramProbeTests.cs \
OrchestratorIDE/Core/Runtime/RuntimeOrchestrator.csLength of output: 9393 🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline OrchestratorIDE/Core/Runtime/NativeVramProbe.cs --items all --view expanded
ast-grep outline OrchestratorIDE.Avalonia/MainWindow.axaml.cs --items all --view expanded --match 'TryBuildNativeHiveBudget'
nl -ba OrchestratorIDE/Core/Runtime/NativeVramProbe.cs | sed -n '1,180p'
nl -ba OrchestratorIDE.Avalonia/MainWindow.axaml.cs | sed -n '2280,2355p'
fd --type f --glob '*.csproj' . | sort | while IFS= read -r project; do
if rg -q 'OrchestratorIDE.NativeRuntime|TargetFramework|TargetFrameworks' "$project"; then
echo "===== $project ====="
rg -n -C 2 '<TargetFrameworks?>|OrchestratorIDE\.NativeRuntime' "$project" || true
fi
done
rg -n -C 3 '\bQueryTimeout\b|\bTask\.WaitAll\s*\(|\bWaitForExitAsync\s*\(' \
OrchestratorIDE/Core/Runtime/NativeVramProbe.csLength of output: 1480 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '===== OrchestratorIDE/Core/Runtime/NativeVramProbe.cs ====='
awk '{ printf "%6d %s\n", NR, $0 }' OrchestratorIDE/Core/Runtime/NativeVramProbe.cs | sed -n '1,180p'
echo '===== OrchestratorIDE.Avalonia/MainWindow.axaml.cs (budget method) ====='
awk '{ printf "%6d %s\n", NR, $0 }' OrchestratorIDE.Avalonia/MainWindow.axaml.cs | sed -n '2280,2355p'
echo '===== target frameworks / NativeRuntime references ====='
fd --type f --extension csproj . | sort | while IFS= read -r project; do
if rg -q '<TargetFramework|<TargetFrameworks|OrchestratorIDE\.NativeRuntime' "$project"; then
echo "----- $project -----"
rg -n -C 2 '<TargetFrameworks?>|OrchestratorIDE\.NativeRuntime' "$project" || true
fi
done
echo '===== related timeout/wait occurrences ====='
rg -n -C 4 '\bQueryTimeout\b|\bTask\.WaitAll\s*\(|\bWaitForExitAsync\s*\(' \
OrchestratorIDE/Core/Runtime/NativeVramProbe.csLength of output: 19524
No further issues found in this follow-up review. 🐇✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
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>
* 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 the live-budget half of Phase B of the native runtime spec. Ships the well-grounded half of Phase B now; explicitly defers the other half rather than shipping an ungrounded generalization (see below).
Please review before merging — same discipline as Phase A (#70): opt-in native runtime, but this changes what admission decisions are actually based on.
What ships
New
NativeVramProbe.TryQueryLiveNvidiaBudget()— a livenvidia-smisubprocess query (memory.total,memory.used), reusing the exact subprocess-CSV pattern already proven safe inOrchestratorSetup/Services/HardwareDetector.QueryNvidiaSmi, but for live current usage instead of a one-time install-time total.The gap this closes: before this PR, there was no "how much VRAM is free right now" query anywhere in the running app. The only VRAM detection that existed lives in a different project (
OrchestratorSetup, the installer), runs once at install time, and only measures total capacity — never live availability. That's exactly whyMainWindow/HiveService's budget providers hardcodedReservedBytes: 0.Wiring:
MainWindowandHiveServicenow pass the budget-building method itself asbudgetProvider, not a closed-over one-time snapshot —RuntimeOrchestrator.EnsureAdmittedalready calls_budgetProvider()fresh on every admission (confirmed from Phase A), so this makes admission decisions genuinely live instead of stale-at-construction-time. Both sites fall back to the pre-Phase-B static-total behavior when the live probe is unavailable (non-NVIDIA GPU,nvidia-smimissing) — never worse than before, only ever more accurate.Small correctness fix along the way: widened
Func<VramBudget>?toFunc<VramBudget?>?inRuntimeOrchestrator/IRoleRuntime. The declared type didn't match reality —EnsureAdmittedalready null-coalesces/throws on a null result andGetReservationSnapshotalready null-checks it — and passing a genuinely nullable-returning method directly surfaced a realCS8621nullability warning that the oldbudget is null ? null : () => budgetpattern had been silently working around.What's deliberately deferred (and why)
The spec's Phase B also calls for a "KV/rs-cache-aware cost estimate" on
OrcScheduler.EstimateRequiredBytes(currently just GGUF file size). I investigated addingAdapterManager.SequenceHardLimit * ~50MB/slotas a fixed reservation — that number is already documented inAdapterManager.cs's own comments, empirically confirmed on real hardware.Rejected: that overhead is specific to hybrid/recurrent-architecture models (Qwen3.5's Gated Delta Net layers) — it does not apply to plain-transformer models, which are the common case.
RuntimeModelAssethas no architecture metadata to distinguish the two. A flat, always-on addition would over-reserve VRAM for every admission regardless of what's actually being loaded, risking real regressions — denying legitimate admissions on modest-VRAM boxes that used to fit fine. It would also break severalOrcSchedulerTeststhat assert exact-fit admission.Shipping a plausible-looking but ungrounded generalization from one empirical data point felt worse than shipping the well-grounded half now and leaving this explicitly open. Will need real GGUF architecture metadata (not currently exposed anywhere) to do properly.
Verification
OrchestratorIDE.UnitTestssuite: 596 passed, 0 failed, 4 skipped (same pre-existingTHEORC_TEST_GGUF-gated skips, unaffected by this change).NativeVramProbe— tolerant of "no GPU" environments (asserts invariants when non-null, doesn't require a GPU to pass).nvidia-smireading taken beforehand (~15.9 GB total, ~3.7 GB used, ~12.2 GB available) — this is a genuine/verify, not just a passing unit test.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests