fix(interp): harden heap ownership and cycle collection by siyul-park · Pull Request #143 · siyul-park/minivm

siyul-park · 2026-07-13T00:50:47Z

Changes Made

correct heap ownership for Alloc, Push, SetGlobal, SetLocal, and Store, including validated refs, unique concrete-pointer ownership, same-value no-ops, and deterministic finalization
replace the explicit root registry with RC-derived trial deletion while preserving stable heap indices and iterative release
preserve exact constant-root and nested-edge counts during interpreter construction and Reset
add regression coverage for host-owned refs, self/multi-object cycles, duplicate edges, dead-to-live edges, finalizers, constant graphs, adaptive collection, and reset pacing
add live-set-based GC pacing with a 64-slot minimum runway, hard-limit clamping, and one collection per allocation attempt
consolidate adaptive, backing-capacity, and hard-limit collection into one allocation flow
update the memory model, host integration, and architecture documentation

Related Issues

Follow-up to Complete the tracing/cycle GC to stop cycle leaks #86
Follow-up to Audit suspected RC ownership invariant violations #103

Additional Information

Self-review found that duplicate nested constant edges could produce a negative residual count and allow a live constant child to be reclaimed; recount now rebuilds exact baseline counts after construction and reset, and the collector rejects negative residual counts before marking.
Follow-up review found that Store could shallow-alias another heap slot, duplicate finalizers, and undercount nested refs; different-address refs and already-owned concrete pointers are now rejected, while Alloc(existingRef) remains the explicit sharing path.
SetGlobal and SetLocal now reject invalid heap refs before mutating or releasing the existing slot.
P3 pool shedding, public pacing controls, and GC statistics are intentionally out of scope.
Verification:
- make lint
- go test -count=1 -race ./...
No throughput benchmark claim is made; P2 bounds cycle retention and avoids repeated collection from small live sets.

Summary by CodeRabbit

Documentation
- Clarified VM heap ownership, host interaction rules, and value lifetime across Reset/Close.
- Updated the memory model to use exact reference counting and a trial-deletion mark-and-sweep cycle collector.
Bug Fixes
- Improved reference lifecycle correctness for SetGlobal, SetLocal, Store, and Alloc, including self-assignment no-ops and proper release/finalization on replacement.
- Reworked cycle collection and heap/Reset behavior for more reliable reclamation.
Tests
- Expanded coverage for heap limits, Reset finalization, ownership transfer, and multiple cycle-collection regression scenarios.

coderabbitai · 2026-07-13T00:51:08Z

📝 Walkthrough

Walkthrough

The interpreter now uses exact reference counting with trial-deletion cycle collection. Heap ownership, reset behavior, marshaling cleanup, cloned state, host-integration guidance, architecture documentation, and liveness tests were updated accordingly.

Changes

Memory lifecycle

Layer / File(s)	Summary
Heap representation and ownership baseline `interp/interp.go`, `docs/architecture.md`, `docs/memory-model.md`	Heap state includes `trial` and `work` buffers; constant boxing and `recount` establish exact reference-count baselines and document ownership edges.
Heap ownership operations `interp/interp.go`, `docs/host-integration.md`, `docs/memory-model.md`	Global, local, store, allocation, retention, release, and replacement paths handle liveness validation, identical assignments, ownership transfer, and slot disposal.
Reset and trial-deletion collection `interp/interp.go`, `interp/interp_test.go`, `docs/memory-model.md`	Reset finalizes dynamic values and rebuilds heap state; collection scans exact counts, marks survivors, sweeps cycles, and validates pacing, liveness, and reuse behavior.
Marshaling and cloned interpreter state `interp/marshal.go`, `interp/trace.go`	Marshaling removes explicit root tracking and cleanup, while cloned interpreters clear transient GC and reference buffers.
Lifecycle validation and runtime documentation `interp/interp_test.go`, `docs/host-integration.md`, `docs/memory-model.md`	Tests cover ownership, finalization, reset, heap limits, and cycle collection; documentation describes updated runtime and host-lifetime semantics.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Interpreter
  participant TraceableRefs
  participant Heap
  Interpreter->>Heap: copy exact rc into trial
  Interpreter->>TraceableRefs: read heap-to-heap Refs edges
  TraceableRefs-->>Interpreter: return reference edges
  Interpreter->>Heap: mark externally reachable objects
  Interpreter->>Heap: sweep unreachable cycles

Possibly related issues

Lock-Free Parallel Workers over a Shared Heap (Pool-integrated) #141: The exact reference-counting and trial-deletion changes implement the issue’s shared-heap ownership and collection direction, although concurrent-worker support is outside this change.

Possibly related PRs

siyul-park/minivm#106: Modifies the interpreter’s reference-counting and garbage-collection machinery in the same runtime area.
siyul-park/minivm#107: Updates the same runtime documentation areas for heap ownership and reference-counting semantics.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately reflects the main change: hardening interpreter heap ownership and cycle collection.
Description check	✅ Passed	The description follows the template and covers changes made, related issues, additional context, and verification.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feature/memory-management

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-07-13T00:59:39Z

Codecov Report

❌ Patch coverage is 88.82682% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.13%. Comparing base (0978e9d) to head (a5f310b).

Files with missing lines	Patch %	Lines
interp/interp.go	89.14%	10 Missing and 9 partials ⚠️
interp/marshal.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
+ Coverage   26.98%   27.13%   +0.14%     
==========================================
  Files          83       83              
  Lines       59196    59275      +79     
==========================================
+ Hits        15974    16082     +108     
+ Misses      41999    41979      -20     
+ Partials     1223     1214       -9

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

interp/interp.go (1)
634-656: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Retain nested refs when Store aliases a heap object

Store turns BoxRef(ref) into i.heap[ref], so writing a traced heap value into another slot adds a second owner without increasing the children’s refcounts. That can drop shared children to zero too early. Retain the aliased value’s refs before assigning it.
🤖 Prompt for 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.

In `@interp/interp.go` around lines 634 - 656, Update Store’s types.Boxed KindRef
handling so that when val aliases i.heap[ref], it retains all references owned
by the aliased heap value before assigning it to i.heap[addr]. Preserve the
existing validity checks, self-assignment fast path, and cleanup of the old
value; use the existing reference-retention mechanism rather than manually
adjusting child counts.

🤖 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 `@interp/interp_test.go`:
- Around line 3224-3237: Add a subtest alongside “ignores same-address
reference” that allocates separate live values, stores BoxRef(otherAddr) at
addr, and asserts the expected child-reference ownership and finalization counts
after releasing or closing the relevant addresses. Use the existing
trackedValue, Alloc, Store, Load, and cleanup patterns to verify the
different-address aliasing path in Store.

---

Outside diff comments:
In `@interp/interp.go`:
- Around line 634-656: Update Store’s types.Boxed KindRef handling so that when
val aliases i.heap[ref], it retains all references owned by the aliased heap
value before assigning it to i.heap[addr]. Preserve the existing validity
checks, self-assignment fast path, and cleanup of the old value; use the
existing reference-retention mechanism rather than manually adjusting child
counts.

🪄 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: b19ba8fa-904b-42c9-9bcc-f5c05e151615

📥 Commits

Reviewing files that changed from the base of the PR and between 0978e9d and e8009b9.

📒 Files selected for processing (7)

docs/architecture.md
docs/host-integration.md
docs/memory-model.md
interp/interp.go
interp/interp_test.go
interp/marshal.go
interp/trace.go

coderabbitai

🧹 Nitpick comments (1)

interp/interp_test.go (1)
3693-3731: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the pacing constants driving these GC-timing assertions.

The thresholds (capacity - 2, the 62-alloc pre-fill, and the 65th alloc that flips cycle.closed from 0→1) are derived from the internal heapRunway/pace() schedule but carry no explanation. Any future tweak to the adaptive goal will make these assertions fail with no hint at why the numbers were chosen. A short comment tying the counts to the goal/runway math would keep the intent legible. The same undocumented-constant pattern repeats in the sibling subtests ("paces from live set" Lines 3770-3776 and "resets adaptive goal" Line 3805).

Setup is intentionally inlined per subtest, which matches the test-style guideline — this is only about annotating the magic numbers, not restructuring.
🤖 Prompt for 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.

In `@interp/interp_test.go` around lines 3693 - 3731, Add concise comments in the
adaptive-GC timing assertions, including the “collects cycles at adaptive goal”
and sibling subtests “paces from live set” and “resets adaptive goal.” Explain
how capacity - 2, the 62-allocation prefill, and the final 65th allocation
derive from the heapRunway/pace() schedule and adaptive-goal transitions,
without restructuring the inlined setup.
Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@interp/interp_test.go`:
- Around line 3693-3731: Add concise comments in the adaptive-GC timing
assertions, including the “collects cycles at adaptive goal” and sibling
subtests “paces from live set” and “resets adaptive goal.” Explain how capacity
- 2, the 62-allocation prefill, and the final 65th allocation derive from the
heapRunway/pace() schedule and adaptive-goal transitions, without restructuring
the inlined setup.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c2432c92-1449-4446-a3ce-32097fbd566e

📥 Commits

Reviewing files that changed from the base of the PR and between e8009b9 and a03de24.

📒 Files selected for processing (3)

docs/memory-model.md
interp/interp.go
interp/interp_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

docs/memory-model.md
interp/interp.go

siyul-park added 3 commits July 13, 2026 09:23

fix(interp): correct heap ownership lifecycle

bfa6e19

refactor(interp): derive cycle roots from refcounts

c102d24

fix(interp): preserve constant ownership across gc

e8009b9

coderabbitai Bot reviewed Jul 13, 2026

View reviewed changes

Comment thread interp/interp_test.go

siyul-park added 2 commits July 13, 2026 10:25

perf(interp): add adaptive gc pacing

bbee243

docs: document adaptive gc pacing

a03de24

coderabbitai Bot reviewed Jul 13, 2026

View reviewed changes

siyul-park added 2 commits July 13, 2026 11:06

fix(interp): enforce unique heap ownership

36c3bfd

refactor(interp): simplify gc allocation pacing

a5f310b

siyul-park merged commit a2cee44 into main Jul 13, 2026
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(interp): harden heap ownership and cycle collection#143

fix(interp): harden heap ownership and cycle collection#143
siyul-park merged 7 commits into
mainfrom
feature/memory-management

siyul-park commented Jul 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 13, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

codecov Bot commented Jul 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

siyul-park commented Jul 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes Made

Related Issues

Additional Information

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

codecov Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

siyul-park commented Jul 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 13, 2026 •

edited

Loading

codecov Bot commented Jul 13, 2026 •

edited

Loading