feat(vm): add maxSteps execution budget#2
Conversation
|
@darylcecile Ready for review! |
There was a problem hiding this comment.
Pull request overview
This PR adds a cooperative maxSteps execution budget to the VM’s public API so hosts can limit interpreted work in addition to the existing wall-clock guard. It extends the VM resource-limiting model, updates docs, and adds basic regression coverage.
Changes:
- Add
maxStepsto VM execution rule types and per-call evaluation/import options. - Thread the step budget into evaluation, import, URL-evaluation, and handle execution contexts.
- Document step budgets in the README and add a VM test for step exhaustion.
Show a summary per file
| File | Description |
|---|---|
src/vm.ts |
Adds the new maxSteps API surface, normalizes it, and passes it into VM execution contexts. |
test/vm.test.ts |
Adds coverage for step-budget exhaustion through vm.eval(...). |
README.md |
Documents maxSteps in the VM options and resource-limits sections. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 5
| expectVMFailure( | ||
| await overrideVm.eval("let i = 0; while (true) { i += 1; }", { maxSteps: 5 }), |
| const maxSteps = normalizeMaxSteps( | ||
| options.maxSteps ?? this.#executionRules.maxSteps, | ||
| ); | ||
| const rootContext = this.#createEvaluationContext(timeLimit, maxSteps); |
| return this.evaluate(response.bodyText, { | ||
| sourceType: options.sourceType, | ||
| timeLimit: options.timeLimit, | ||
| maxSteps: options.maxSteps, |
| if (!Number.isSafeInteger(value) || value < 0) { | ||
| throw new VMError( | ||
| VMErrorCode.VMRuntimeError, | ||
| "executionRules.maxSteps must be a non-negative safe integer.", |
|
|
||
| Therefore time limits are guardrails, not hard CPU guarantees. A synchronous host capability can still block while it runs, and there is currently no independent memory limit, public instruction counter, scheduler, or deterministic event loop. | ||
|
|
||
| `executionRules.maxSteps` and per-call `evaluate(source, { maxSteps })` provide a cooperative step budget: |
|
See suggestions from the copilot review above. Also, im not sure |
|
@darylcecile I've integrated the suggestions from Copilot and your feedback. |
|
Hey @darylcecile, had a chance to look at the PR? Everything is up to date on my end |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/vm.ts:1002
- Deleting the module record from
#moduleGraphon any evaluation failure means a module that throws (or has side effects before throwing) will be reloaded and re-executed on subsequentvm.import()calls, which diverges from typical ES module caching semantics and can cause repeated side effects / extra work. If the goal is specifically to allow retries after budget exhaustion, consider only deleting the record for budget-related failures (e.g.VM_STEPS_EXCEEDED_ERROR/VM_TIMEOUT_ERROR) and caching failures for other runtime errors.
// If evaluation failed for any reason, remove the partially-created
// module record from the graph so callers can retry with a different
// execution budget or after a reset. This avoids permanently
// caching exhausted/failed module records.
graph.delete(record.specifier);
if (record.status !== "evaluated") {
record.status = "linked";
}
throw error;
- Files reviewed: 7/7 changed files
- Comments generated: 2
| @@ -795,7 +861,10 @@ export class VM { | |||
| ); | |||
| } | |||
|
|
|||
| if (maxBytes !== undefined && utf8ByteLength(response.bodyText) > maxBytes) { | |||
| if ( | |||
| maxBytes !== undefined && | |||
| utf8ByteLength(response.bodyText) > maxBytes | |||
| ) { | |||
| return failure( | |||
| new VMError( | |||
| VMErrorCode.VMSecurityError, | |||
| @@ -808,6 +877,7 @@ export class VM { | |||
| return this.evaluate(response.bodyText, { | |||
| sourceType: options.sourceType, | |||
| timeLimit: options.timeLimit, | |||
| maxSteps: options.maxSteps, | |||
| }); | |||
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
README.md:1
- Two documentation issues on the updated lines: (1) the example code block mixes tabs and spaces on the new
maxStepsline, which breaks alignment in the rendered snippet; and (2) the API snippet statescapabilities.executionRulesexists while also listing a top-levelexecutionRulesalias—please ensure this matches the actualVMOptionsshape implemented insrc/vm.ts(ifcapabilities.executionRulesis not supported, it should be removed from the README; if it is supported, the implementation should reflect it consistently).
# @catmint-fs/jsvm
| function isVMErrorRecord( | ||
| error: unknown, | ||
| ): error is { readonly code: VMErrorCode; readonly message: string; readonly details: VMErrorDetails } { | ||
| return ( | ||
| isPlainObject(error) && | ||
| isVMErrorCode(error.code) && | ||
| typeof error.message === "string" && | ||
| isPlainObject(error.details) | ||
| ); | ||
| } |
|
Hey @darylcecile, had a chance to look at the PR? Everything is up to date on my end |
|
hey @Dayifour great job on this - however the extra formatting (likely coming from a formatter extension in your IDE or from your AI tooling) makes reviewing this PR difficult. Any chance you can clean up any extra formatting so that only relevant changes are left? This will also ensure formatting conformance across the rest of the codebase. I have an in-flight PR to add formatting rules to the repo - but for now lets clean this up and then i'll stamp LGTM 🚀 |
|
Hey @darylcecile, I totally understand that extra formatting noise really does make diffs harder to follow. I've done my best to clean it up, but I'm struggling to fully isolate where it's coming from. I'll keep investigating on my end. In the meantime, if the remaining changes are clear enough, I'm happy to wait for your formatting PR to land and align with it then. Let me know how you'd like to proceed! |
|
@Dayifour sorry for the delay - please rebase your changes onto the default branch. Ive now set up formatting rules |
Set up oxfmt 0.52 as the project's formatter so contributor PRs cannot silently drift from the existing code style. Rules were tuned against PR darylcecile#2, which contained large amounts of formatting noise (collapsed object literals re-expanded to multi-line, reordered imports) on top of real changes. - .oxfmtrc.json: 2-space, double quotes, semicolons, trailing commas, printWidth 100, objectWrap=collapse (defeats magic-trailing-comma expansion), sortImports=true (canonical import statement order). - package.json: format and format:check scripts; oxfmt devDependency. - AGENTS.md: documents the format commands and the no-hand-reformat rule. - .github/workflows/format-pr.yml: runs bun run format on every same-repo PR and pushes the result back as a github-actions[bot] commit. Gated on head.repo == github.repository because fork PRs only get a read-only GITHUB_TOKEN; fork contributors run format locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n steps-exceeded error, tighten test
…tion after budget exhaustion
Set up oxfmt 0.52 as the project's formatter so contributor PRs cannot silently drift from the existing code style. Rules were tuned against PR darylcecile#2, which contained large amounts of formatting noise (collapsed object literals re-expanded to multi-line, reordered imports) on top of real changes. - .oxfmtrc.json: 2-space, double quotes, semicolons, trailing commas, printWidth 100, objectWrap=collapse (defeats magic-trailing-comma expansion), sortImports=true (canonical import statement order). - package.json: format and format:check scripts; oxfmt devDependency. - AGENTS.md: documents the format commands and the no-hand-reformat rule. - .github/workflows/format-pr.yml: runs bun run format on every same-repo PR and pushes the result back as a github-actions[bot] commit. Gated on head.repo == github.repository because fork PRs only get a read-only GITHUB_TOKEN; fork contributors run format locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9802727 to
5d80145
Compare
|
@darylcecile Finally, i think it's clear enough now :) |
|
Merged! thanks for the contribution |
Add maxSteps execution budget to VM API
Summary
maxStepsinexecutionRulesand per-call optionsTesting
bun testbun run typecheck