fix: Safely reset wasmtime memory per instance reset#157
Merged
AndrewSisley merged 5 commits intoJun 3, 2026
Conversation
Drives 2000 Reset() cycles; asserts linear memory growth <= 2 pages.
…work#155) Drops and recreates wasmtime.Store on Reset(); gives dlmalloc a clean heap.
Keeps per-developer design/drawback notes out of the repo.
Dropped stores hold C memory invisible to Go GC; explicit Close keeps host RSS flat (~127KB/cycle -> ~5KB/cycle).
wazero/js runtimes still leak on Reset; the test only covers wasmtime.
📝 WalkthroughWalkthroughThis PR fixes memory exhaustion in the Wasmtime runtime by switching from a single long-lived shared Store to creating a fresh Store per module instance. A Memory interface extension enables size reporting across all runtimes, and a regression test validates that repeated ChangesWasmtime Memory Leak Fix via Per-Instance Store Management
Sequence Diagram(s)sequenceDiagram
participant Client
participant wRuntime
participant Engine
participant InstanceHandles
participant Store
participant Module
Client->>wRuntime: NewModule(moduleBytes)
wRuntime->>Engine: NewModule(engine, moduleBytes)
Engine-->>wRuntime: Compiled Module
Client->>wRuntime: NewInstance()
wRuntime->>InstanceHandles: newInstanceHandles(module)
InstanceHandles->>Store: Create fresh Store
Store-->>InstanceHandles: Store ready
InstanceHandles->>Module: Instantiate with Store
Module-->>InstanceHandles: Instance + Exports (memory, alloc, transform)
InstanceHandles-->>wRuntime: handles (Store, memory, alloc, fn, instance)
wRuntime-->>Client: module.Instance (backed by handles)
Client->>Client: Use instance.Transform(), instance.Alloc(), etc.
Client->>Client: Call instance.Reset()
Client->>InstanceHandles: Reset
InstanceHandles->>InstanceHandles: newInstanceHandles(module) again
InstanceHandles->>Store: Create new Store
InstanceHandles->>InstanceHandles: Swap handles in-place
InstanceHandles->>Store: Close old Store
Store-->>InstanceHandles: Old Store freed
InstanceHandles-->>Client: Reset complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Out-of-scope changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@host-go/engine/module/memory.go`:
- Around line 14-15: The Memory.Size() API currently returns uint32 which
overflows at the wasm32 maximum (4,294,967,296 bytes); change the Memory.Size()
signature to return uint64 and update all implementations (e.g.,
BytesMemory.Size(), the js implementation returning m.array.Length(), and the
wazero adapter that uses m.memory.Size() * 65536) to return uint64 values (e.g.,
uint64(len(s.data)), uint64(m.array.Length()), and uint64(m.memory.Size()) *
65536) so the full 4GiB range is representable without wraparound; update any
callers to expect uint64 as well.
In `@host-go/engine/tests/wasm32_pipeline_reset_test.go`:
- Around line 69-70: The subtraction "growth := final - baseline" can underflow
when final < baseline; update the calculation (using the variables final,
baseline, growth and the call instance.Memory().Size()) to compute the delta in
a signed type or clamp negative results to zero so that any shrink becomes 0
rather than a large wraparound value; modify the test to compute a signed
difference or use a saturating subtraction and assign the non-negative result to
growth.
In `@host-go/runtimes/wasmtime/runtime.go`:
- Around line 71-154: newInstanceHandles currently calls wasmtime.NewStore(eng)
to create store but returns early on many errors without closing it; ensure the
created store is closed on all error paths but remains open when returning the
successful *instanceHandles. Modify newInstanceHandles to arrange cleanup (e.g.,
set a defer that calls store.Close() conditionally or use a small local cleanup
function/flag) so any early return (errors from export/function lookups,
json.Marshal, alloc.Call, pipes.WriteItem, setParam.Call, pipes.ReadItem, etc.)
invokes store.Close(), and ensure the defer/flag is disabled before the final
return that constructs and returns the instanceHandles (store, memory, alloc,
transform).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a102183c-89cb-47e5-bfff-b360b88f5da1
📒 Files selected for processing (6)
.gitignorehost-go/engine/module/memory.gohost-go/engine/tests/wasm32_pipeline_reset_test.gohost-go/runtimes/js/memory.gohost-go/runtimes/wasmtime/runtime.gohost-go/runtimes/wazero/memory.go
📜 Review details
🔇 Additional comments (1)
.gitignore (1)
29-29: LGTM!
AndrewSisley
changed the title
AndrewSisley
approved these changes
Jun 3, 2026
Collaborator
There was a problem hiding this comment.
Looks good to me! Thank you very much, this is very handy. Let me know if you also wish to resolve this for the other runtimes (no pressure at all) #159
| // The GC finalizer would do this eventually, but wasm memory lives outside the Go heap | ||
| // and exerts no pressure on the Go GC, so dropped stores pile up faster than they are | ||
| // collected (issue #155). | ||
| oldStore.Close() |
Collaborator
There was a problem hiding this comment.
praise: Thank you for this!
| return r.(module.MemSize), err | ||
| }, | ||
| Transform: func(next func() module.MemSize) (module.MemSize, error) { | ||
| if resetErr != nil { |
Collaborator
There was a problem hiding this comment.
praise: Thanks for working around the lack of error return from Reset and still bubbling the error back up to the user. Reset can be reformed later.
| // | ||
| // Without the store-per-instance fix, dlmalloc-rs bookkeeping is overwritten on each Reset, | ||
| // causing memory.grow to be called every cycle and leak ~64 KiB per call (issue #155). | ||
| func TestWasm32ResetDoesNotLeakMemory(t *testing.T) { |
Collaborator
There was a problem hiding this comment.
praise: Thank you very much for including this in the PR.
Contributor
Author
|
Yeah I can get on that |
This was referenced Jun 3, 2026
4 tasks
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
2 participants
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.
Relevant issue(s)
Resolves #155
Description
The wasmtime runtime leaks wasm linear memory on every
Reset()call, eventually crashing the host process under a large set of inputs.Fix (store-per-instance + re-instantiate on Reset):
wRuntimenow holds only the sharedwasmtime.Engine(so module compilation is still reused); eachmodule.Instanceowns its ownwasmtime.Store.Reset()creates a fresh store, re-instantiates the module, re-appliesset_param, and swaps it in behind a shared*instanceHandlespointer so theAlloc/Transform/Memoryclosures see the new store transparently.Close()d. Relying on Go GC finalizers is not enough: the store's linear memory is C-allocated and invisible to Go's GC pressure heuristics, so dropped stores pile up faster than they are collected. Measured host RSS growth without the explicit close was ~127 KiB per Reset cycle; with it, RSS stays flat.module.Memorygained aSize()method. Note this is a compile-time breaking change for any external implementers of that interface; all in-repo implementations (wasmtime, wazero, js, BytesMemory) are updated.Tasks
How has this been tested?
TestWasm32ResetDoesNotLeakMemory(host-go/engine/tests/wasm32_pipeline_reset_test.go): drives a single instance through 2,000 Append/Reset cycles and asserts wasm linear memory growth stays within 2 pages after a 50-cycle warm-up.ps), since the in-tree test cannot observe memory held by dropped stores:Close(): ~127 KiB/cycle RSS growth (linear).Close(): flat (~5 KiB/cycle, amortizing to zero).host-gotest suite (make test),go vet,gofmt, and cross-compilation forGOOS=js GOARCH=wasmandGOOS=windowsall pass.Specify the platform(s) on which this was tested: