test(risor,starlark): regression test for Eval cancellation timing#127
Merged
Conversation
Closes #125 PR #81 added `context.AfterFunc(ctx, thread.Cancel)` to Starlark's exec() to fix a goroutine leak, but no test asserted that **cancellation actually halts execution within a bounded time**. Same gap existed for Risor — the Risor v2 VM checks ctx.Done() at DefaultContextCheckInterval (1000 instructions) and via a background goroutine, but nothing locked the behavior in. Extism is already covered by the execHelper cancellation cases added in PR #121. Add TestEval_CancellationHaltsExecution to both Risor and Starlark evaluator test files. The script body is a long-running loop sized so natural completion would far outrun the 2-second test deadline; only ctx cancellation can return Eval early. - Risor: Risor v2 has no for/while statements (it's a functional language with .map/.filter/.each higher-order methods), so the long-running construct is `range(1e9).each(x => x)`. The VM's periodic ctx-check inside .each's callable.Call propagates cancellation. - Starlark: lazy `range(1e12)` with a Python-style for-loop. The AfterFunc-registered thread.Cancel halts at the next instruction boundary. Both tests: 1. Launch Eval in a goroutine. 2. Sleep 50ms so the engine is observably mid-script. 3. Cancel the context. 4. Assert Eval returns within 2s with a cancellation-shaped error (errors.Is(context.Canceled) OR "context canceled" / "cancel" substring — Starlark's thread.Cancel wraps the reason in an EvalError that doesn't unwrap to context.Canceled). Stress-tested locally with `-count=20 -race`: 40 runs, no flakes, ~2 seconds total wall time. Out of scope: Eval-halts-on-deadline (context.WithTimeout); same code path as cancel.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds regression tests verifying that cancelling the context passed to Eval() halts a long-running script within 2 seconds for both the Risor and Starlark engines, locking in the cancellation behavior fixed in PR #81 and the periodic ctx-check in Risor v2.
Changes:
- Add
TestEval_CancellationHaltsExecutionto the Starlark evaluator test file using afor i in range(1e12): passloop. - Add
TestEval_CancellationHaltsExecutionto the Risor evaluator test file usingrange(1e9).each(x => x).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| engines/starlark/evaluator/evaluator_test.go | New cancellation timing regression test for Starlark Eval. |
| engines/risor/evaluator/evaluator_test.go | New cancellation timing regression test for Risor Eval. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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
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
PR #81 added
context.AfterFunc(ctx, thread.Cancel)to Starlark'sexec()to fix a goroutine leak, but no test asserted that cancellation actually halts execution within a bounded time. Same gap existed for Risor — the Risor v2 VM checksctx.Done()atDefaultContextCheckInterval(1000 instructions) and via a background goroutine, but nothing locked the behavior in. Extism is already covered by theexecHelpercancellation cases added in PR #121.Adds
TestEval_CancellationHaltsExecutionto both Risor and Starlark evaluator test files. The script body is a long-running loop sized so natural completion would far outrun the 2-second test deadline; only ctx cancellation can returnEvalearly.Script bodies
range(1e9).each(x => x).map/.filter/.each). The VM's periodic ctx-check inside.each'scallable.Callpropagates cancellation.for i in range(1e12): passinsidedef spin(): ...thread.Cancelhalts at the next instruction boundary.Test shape
Both tests:
Evalin a goroutine.Evalreturns within 2s with a cancellation-shaped error —errors.Is(context.Canceled)OR a"context canceled"/"cancel"substring (Starlark'sthread.Cancelwraps the reason in anEvalErrorthat doesn't unwrap tocontext.Canceled).Stress test
Locally ran
go test -race -count=20 -run TestEval_CancellationHaltsExecution ./engines/risor/evaluator/... ./engines/starlark/evaluator/...— 40 runs, zero flakes, ~2 seconds total wall time.Out of scope
Eval-halts-on-deadline (context.WithTimeout) — same code path as cancel.Test plan
go test -race -count=1 ./...— full suite greengo test -race -count=20on the two new tests — no flakesgo vet ./...— cleanCloses #125
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code