Skip to content

Fix subinterpreter lifecycle edge cases - #8632

Merged
youknowone merged 1 commit into
RustPython:mainfrom
youknowone:subinterpreter-post-merge-fixes
Aug 31, 2026
Merged

Fix subinterpreter lifecycle edge cases#8632
youknowone merged 1 commit into
RustPython:mainfrom
youknowone:subinterpreter-post-merge-fixes

Conversation

@youknowone

@youknowone youknowone commented Aug 31, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member
  • Closes #xxxx

One of checkbox below must be checked.

  • I did not use AI to write the code of this patch.
  • This PR follows our AI policy

Summary

Follow-up to #8605 addressing valid findings from post-merge review:

  • Preserve the logical descriptor of sliced and cast memoryviews during cross-interpreter transfer.
  • Make repeated channel-end release idempotent.
  • Scope runtime-owned subinterpreter cleanup to the creating top-level runtime.
  • Re-read the owned-interpreter table during shutdown so finalizers cannot leave newly created children behind.
  • Add regressions for memoryview metadata, repeated release, and multiple embedded runtimes.

The behavior was compared against CPython 3.14.7. Two other review suggestions were intentionally not included: CPython has the same EXTENDED_ARG limitation in _PyCode_ReturnsOnlyNone(), and its absolute-path construction also produces the observed double separator for a root working directory.

Testing

  • cargo clippy
  • INSTA_WORKSPACE_ROOT=/Users/youknowone/Projects/RustPython-11 cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher --exclude rustpython-capi
  • (cd crates/capi && cargo test) (103 passed)
  • prek run --from-ref upstream/main --to-ref HEAD
  • cargo test -p rustpython-vm releasing_an_end_twice_is_a_noop
  • cargo test -p rustpython-vm --features threading owned_subinterpreter_cleanup_is_scoped_to_its_runtime
  • cargo run -- extra_tests/snippets/stdlib_subinterpreters.py
  • Direct comparison of the new snippet with CPython 3.14.6

AI assistance

OpenAI Codex (GPT-5) assisted extensively with CPython source comparison, implementation, regression-test design, independent review coordination, validation, commit preparation, and this pull request description. The contributor reviewed the findings and directed which valid issues to address.

Summary by CodeRabbit

  • Bug Fixes
    • Preserved memoryview layout metadata when buffers are cloned or transferred.
    • Prevented repeated channel-end releases from corrupting open-end tracking.
    • Ensured subinterpreter cleanup is isolated to the correct runtime.
  • Tests
    • Added coverage for buffer round-tripping, including strided and cast memoryviews.
    • Added tests for repeated channel-end release and isolated runtime finalization.

@youknowone
Fix post-merge subinterpreter lifecycle issues
ea68d61 
Preserve logical memoryview descriptors across interpreter transfers, make channel-end release idempotent, and scope runtime-owned interpreter cleanup to the creating top-level runtime. Re-read owned interpreters during shutdown so finalizers cannot leave newly-created children behind.

Add regression coverage for sliced and cast memoryviews, repeated channel release, and multiple embedded runtimes.

Assisted-by: Codex:GPT-5
@coderabbitai

coderabbitai Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: db062ad3-1202-4efe-b5db-451a8fd8ee1d

📥 Commits

Reviewing files that changed from the base of the PR and between a68e3b0 and ea68d61.

📒 Files selected for processing (6)
  • crates/vm/src/builtins/memory.rs
  • crates/vm/src/stdlib/_interpchannels.rs
  • crates/vm/src/vm/interpreter.rs
  • crates/vm/src/vm/mod.rs
  • crates/vm/src/vm/runtime.rs
  • extra_tests/snippets/stdlib_subinterpreters.py

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Walkthrough

The changes preserve memoryview descriptor metadata, make channel-end release idempotent, and scope owned subinterpreter cleanup to each runtime. Tests cover buffer round-tripping, repeated release, and isolation between embedded runtimes.

Changes

Runtime and channel correctness

Layer / File(s) Summary
Buffer and channel state handling
crates/vm/src/builtins/memory.rs, crates/vm/src/stdlib/_interpchannels.rs, extra_tests/snippets/stdlib_subinterpreters.py		 clone_buffer copies the memoryview descriptor. release_end ignores repeated releases. Tests cover channel counters and strided and cast memoryview round-trips.
Runtime ownership tracking
crates/vm/src/vm/mod.rs, crates/vm/src/vm/interpreter.rs, crates/vm/src/vm/runtime.rs		 PyGlobalState stores runtime_root_id. Owned interpreter entries store that root id, and runtime queries filter interpreter ids by root.
Scoped subinterpreter finalization
crates/vm/src/vm/interpreter.rs		 Finalization destroys only subinterpreters owned by the current runtime and rereads ownership after each destruction. A test verifies runtime isolation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to ea68d

The PR improves subinterpreter isolation and cleanup, preserves memoryview behavior across interpreters, and makes channel-end release safe to repeat. A shutdown-time finalizer may still create a child after cleanup has completed, allowing that child to outlive its runtime; the change is mergeable with explicit owner awareness or follow-up for this bounded lifecycle risk.

Suggested reviewers: shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise, specific, and accurately summarizes the main changes, which address subinterpreter lifecycle and cleanup edge cases.
Docstring Coverage ✅ Passed Docstring coverage is 81.25% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 6 files.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@codspeed

codspeed Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 36 untouched benchmarks

Comparing youknowone:subinterpreter-post-merge-fixes (ea68d61) with main (a68e3b0)

Open in CodSpeed

@youknowone
youknowone merged commit 399378c into RustPython:main Aug 31, 2026
30 checks passed
@youknowone
youknowone deleted the subinterpreter-post-merge-fixes branch August 31, 2026 23:47
youknowone added a commit that referenced this pull request Sep 16, 2026
@youknowone
Fix post-merge subinterpreter lifecycle issues (#8632)
6aac465 
Preserve logical memoryview descriptors across interpreter transfers, make channel-end release idempotent, and scope runtime-owned interpreter cleanup to the creating top-level runtime. Re-read owned interpreters during shutdown so finalizers cannot leave newly-created children behind.

Add regression coverage for sliced and cast memoryviews, repeated channel release, and multiple embedded runtimes.

Assisted-by: Codex:GPT-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone