Add per-execution memory budgets to RVM - #792
Conversation
Add opt-in live-memory budgets for run-to-completion RVM evaluations with typed Rust, FFI, and C# failures. Preserve the process-global limit as a separate safeguard and reject budgeted suspendable execution. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ba89dec-e1cf-482a-9f10-c97b107ae6ef
0105542 to
2f2238c
Compare
There was a problem hiding this comment.
Pull request overview
Adds opt-in, per-execution memory budgets to isolate RVM evaluations using thread-local allocator accounting.
Changes:
- Enforces fresh memory budgets for run-to-completion RVM execution.
- Adds typed Rust, FFI, and C# errors and configuration APIs.
- Adds documentation, tests, and benchmark coverage.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/memory_limits.rs |
Tests enforcement, precedence, reset, and threading. |
src/utils/limits/mod.rs |
Exports budget configuration and counters. |
src/utils/limits/memory.rs |
Defines memory budget configuration. |
src/rvm/vm/state.rs |
Resets state before capturing baselines. |
src/rvm/vm/rules.rs |
Treats budget exhaustion as fatal. |
src/rvm/vm/machine.rs |
Implements budget accounting and checks. |
src/rvm/vm/execution.rs |
Integrates budgets into execution entry points. |
src/rvm/vm/errors.rs |
Adds typed budget errors. |
src/lib.rs |
Exposes the Rust configuration API. |
mimalloc/src/mimalloc.rs |
Re-exports thread live-byte accounting. |
mimalloc/src/limits.rs |
Implements and tests live-byte sampling. |
mimalloc/src/lib.rs |
Exposes allocator accounting publicly. |
docs/limits/memory_budget.md |
Documents behavior and limitations. |
bindings/ffi/src/rvm.rs |
Adds FFI configuration and status mapping. |
bindings/ffi/src/limits.rs |
Defines FFI budget configuration. |
bindings/ffi/src/common.rs |
Adds the FFI exhaustion status. |
bindings/csharp/Regorus/StatusExtensions.cs |
Maps exhaustion to a typed exception. |
bindings/csharp/Regorus/Rvm.cs |
Adds budget configuration methods. |
bindings/csharp/Regorus/RegorusMemoryBudgetExceededException.cs |
Defines the typed exception. |
bindings/csharp/Regorus/NativeMethods.cs |
Adds native declarations and types. |
bindings/csharp/Regorus/MemoryBudgetConfig.cs |
Defines validated C# configuration. |
bindings/csharp/Regorus.Tests/RvmMemoryBudgetTests.cs |
Tests the C# API. |
bindings/csharp/README.md |
Documents C# usage. |
benches/rvm_benchmark.rs |
Benchmarks budget overhead. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Maksym (@maksym-mishchenko) Thanks for doing this very useful feature! Overall looks good to me. Copilot reviews found some interesting cases that are worth addressing. |
Tighten run-to-completion accounting, reject budgeted resume, include FFI serialization, and expose typed binding failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ba89dec-e1cf-482a-9f10-c97b107ae6ef
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ba89dec-e1cf-482a-9f10-c97b107ae6ef
Hi Anand Krishnamoorthi (@anakrish), thanks for the thorough review. I addressed comments, I kept the two API suggestions unchanged for the reasons explained in their threads. Could you please take another look when you have time? |
| } | ||
|
|
||
| #[cfg(feature = "allocator-memory-limits")] | ||
| vm.set_memory_budget_config(config.memory_budget.then(|| MemoryBudgetConfig { |
There was a problem hiding this comment.
Low: These benchmark imports/calls use #[cfg(feature = "allocator-memory-limits")], but the core MemoryBudgetConfig export and RegoVM::set_memory_budget_config are gated by all(feature = "allocator-memory-limits", not(miri)). A bench build selected under Miri can therefore fail to compile.
Suggested change:
#[cfg(all(feature = "allocator-memory-limits", not(miri)))]
use std::num::NonZeroU64;
#[cfg(all(feature = "allocator-memory-limits", not(miri)))]
use regorus::MemoryBudgetConfig;
#[cfg(all(feature = "allocator-memory-limits", not(miri)))]
vm.set_memory_budget_config(config.memory_budget.then(|| MemoryBudgetConfig {
limit: NonZeroU64::new(MEMORY_LIMIT_BYTES).expect("non-zero memory budget"),
}));Please use the same predicate for both imports and the configuration block.
| let result = RegorusResult::ok_string(json); | ||
|
|
||
| #[cfg(all(feature = "allocator-memory-limits", not(miri)))] | ||
| if let Err(err) = guard.check_memory_budget() { |
There was a problem hiding this comment.
Medium: If this post-serialization budget check fails, the FFI returns MemoryBudgetExceeded after dropping only the provisional JSON result, but the VM has already stored the value as ExecutionState::Completed { result }. A caller can then call regorus_rvm_get_execution_state() and observe/re-serialize the oversized completed result despite the reported failure.
Suggested shape:
if let Err(err) = guard.check_memory_budget() {
regorus_result_drop(result);
guard.mark_execution_error(err.clone()); // clear retained result/state
return Err(err.into());
}Alternatively, move the final serialization check into a core-owned completion helper that transitions the VM to ExecutionState::Error and releases the retained result before returning. Please add a regression asserting the state is Error after this failure.
| .jump_to(0_u32) | ||
| .map_err(|err| self.apply_memory_budget_precedence(err)) | ||
| .and_then(|value| { | ||
| self.check_memory_budget()?; |
There was a problem hiding this comment.
Medium: When this final budget check fails, the Err arm records ExecutionState::Error but leaves the failed execution allocations (registers, rule_cache, evaluated, and pooled values) resident until the next execution or VM drop. The named/indexed entry-point paths and the FFI post-marshalling failure do not clean them up at all. Reusing a VM after repeated oversized failures can therefore accumulate roughly one failed result/state per cycle.
Suggested shape:
fn fail_execution(&mut self, err: VmError) -> VmError {
self.release_previous_execution_state();
self.execution_state = ExecutionState::Error { error: err.clone() };
err
}Use this helper in the run-to-completion Err arm, the named/indexed entry-point budget-error paths, and the FFI post-marshalling failure path. Preserve the error value while releasing the result/register/cache state, and add a regression that repeats budget failures on a reused VM and verifies memory/state are reset.
The rationale for both sounds good. Review rerun found 3 more comments worth addressing. Then it should be good to go. |
|
Maksym (@maksym-mishchenko) I see the following drawbacks in this implementation:
Why haven't you pinned thread level baseline and used every allocation (delta gated) for comparison (just like a global limit)? Why have you chosen execute() calls? Consider whether the intended primitive is a scope ( Anand Krishnamoorthi (@anakrish) any thoughts on the above ^? Apart from the design: Follow-ups on the fixes from the previous review round
New
|
Mark Birger (@kusha) However, I do see that it would be nice to also limit input, data memory consumption. Some challenges around implementation:
Some ideas
|
Maybe this would be the simplest approach. Have a thread level limit on alive memory? |
Yes. That was also my impression from the PR description and the implementation. An execution-level budget is the clearest semantic: it bounds the additional working memory used during one RVM execution and intentionally excludes objects such as input, data, and the program that may have been created before—and may outlive—that execution. However, I do agree that large data/input may go undetected by this budget, even if they are eventually subject to configured global memory limit. We should meet to discuss your scenario and see if the design can be generalized and the semantics made more clear/easy to reason about. |
Add synchronous scoped accounting across evaluation-specific data, execution, and native result production. Expose matching one-call FFI and C# APIs with terminal cleanup and reuse guarantees. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Mark Birger (@kusha) Anand Krishnamoorthi (@anakrish). I made another pass based on this discussion. The existing I avoided a Program loading and compilation are still excluded. I also left input and context out for now. Is covering data enough for the fetch scenario, or do you think input should be included before we approve the API? The cleanup and documentation issues are fixed as well. The benchmark showed no measurable difference: 859.82 ns without the budget and 847.59 ns with it, with overlapping intervals. |
|
Maksym (@maksym-mishchenko) |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
AB#3522638
A process-global allocator limit cannot isolate individual policy evaluations and may cause unrelated requests to fail. This adds an optional memory budget to each run-to-completion RVM execution.
The budget measures additional current-thread live bytes above a fresh execution baseline. Previous execution-owned values are released before that baseline is captured. Program compilation and data, input, and context loading remain outside the budget.
Budget exhaustion has additive typed Rust, FFI, and C# errors and takes precedence over the process-global limit. Budgeted suspendable execution is rejected because its thread-local accounting cannot safely span thread migration.
The opt-in controls are
RegoVM::set_memory_budget_configand C#Rvm.SetMemoryBudgetConfig; existing behavior is unchanged when no budget is configured.