fix(thread): loud cross-heap transfer errors, worker MallocState cleanup, owner-tagged timer dispatch (#6185)#6212
Merged
Conversation
…anup (#6185) Two mechanical cross-thread GC-safety wins from the 2026-07-09 GC audit §6, independent of the larger single-JS-thread-vs-multi-heap threading-model decision (which remains open). 1. Loud SerializedValue failures (audit P2). `serialize_nanbox_for_thread` silently lowered every non-transferable runtime type (Map, Set, Promise, Error, TypedArray, Buffer, Symbol, Temporal, native handles, unmaterialized lazy JSON arrays) to Inline(TAG_UNDEFINED), so a captured/returned value of one of these crossed a perry/thread boundary as `undefined` with no diagnostic. Now the serializer emits a named SerializedValue::Unsupported marker, and the transfer boundary raises a catchable TypeError on the MAIN thread — capture/element paths (spawn/parallelMap/parallelFilter) throw synchronously from the calling thread, and the spawn RETURN path rejects the returned promise (the worker can't throw: it has no setjmp frame). All currently-working transfers (arrays, plain objects, closures, strings, numbers, BigInt, Date, SharedArrayBuffer) are preserved unchanged. In spawn, captures are serialized+guarded before the promise is allocated so the throw (js_throw longjmps, skipping Rust drops) cannot leak a pinned promise. 2. Drop for MallocState (audit P1 leak). The thread_local MALLOC_STATE tracked worker-allocated promises/maps/errors/large-closures/tracked-buffers with no Drop, so a spawn/parallelMap worker leaked all of them at thread exit (the first malloc-count GC needs 100k objects, so per-request workers never collected once). Added a Drop that reclaims every tracked block via the same dealloc(Layout) path the sweep uses. Sound because TLS Drop only runs at thread teardown, when the worker's heap is private and unreferenced (results already crossed as owned SerializedValue), and the main thread's MALLOC_STATE is not dropped until process teardown. Deliberately does NOT run payload finalizers: those touch other thread-locals (MAP_REGISTRY, PROMISE_CONTEXTS, async-hooks queues) whose TLS-destruction order is unspecified, so reaching a destroyed TLS would panic and abort the thread; external side-allocations are left to the fuller rework. Pinned objects are skipped, mirroring the sweep. Item 3 (owner-ThreadId-tagged timer dispatch) is deferred: the Android UI-thread pump fires timers registered on the perry-native thread by design (perry-ui-android/src/app.rs nativePumpTick; timer.rs module doc), so a naive owner!=current skip would break all Android setTimeout/setInterval delivery. That P0 needs the per-thread-queue rework, not this narrow slice. Tests: perry-runtime --lib (1210 passed). New unit tests cover the serializer's named-Unsupported routing for a real Map, recursion of the boundary check through arrays/objects/closures, preservation of supported transfers, and the MallocState free path (byte total + pinned-skip). Ref #6185; 2026-07-09 GC audit.
📝 WalkthroughWalkthroughThis PR fixes a memory leak in ChangesMallocState Drop Cleanup
Thread Transfer Guard
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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 `@crates/perry-runtime/src/thread.rs`:
- Around line 466-480: The `guard_transferable` checks in `thread.rs` can throw
via `js_throw`, which longjmps and leaves heap-backed buffers like
`serialized_elements`, `caps`, and `all_results` unreleased on the error path.
Update the `spawn`/`parallelMap`/`parallelFilter` call sites to run
`guard_transferable` before allocating those temporary Vecs, or explicitly
drop/free the buffers before any possible throw. Use the existing
`throw_unsupported_transfer` and `guard_transferable` helpers to keep the
validation on the calling thread without retaining allocated state.
🪄 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: 881009c5-8846-4626-a3e9-d68e3baedfba
📒 Files selected for processing (2)
crates/perry-runtime/src/gc/malloc.rscrates/perry-runtime/src/thread.rs
Comment on lines
+466
to
+480
| unsafe fn throw_unsupported_transfer(type_name: &str) -> ! { | ||
| crate::exception::js_throw(make_unsupported_transfer_error(type_name)); | ||
| } | ||
|
|
||
| /// Main-thread guard: if any value in `values` is non-transferable, throw a | ||
| /// named `TypeError`. Call this at a `spawn`/`parallelMap`/`parallelFilter` | ||
| /// serialization boundary, on the calling thread, before spawning any worker. | ||
| /// | ||
| /// # Safety | ||
| /// Same as [`throw_unsupported_transfer`] — main/calling thread only. | ||
| unsafe fn guard_transferable(values: &[SerializedValue]) { | ||
| if let Some(name) = values.iter().find_map(first_unsupported_transfer_type) { | ||
| throw_unsupported_transfer(name); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file with line numbers around the cited areas.
sed -n '430,520p' crates/perry-runtime/src/thread.rs
printf '\n--- around 830-990 ---\n'
sed -n '830,990p' crates/perry-runtime/src/thread.rs
printf '\n--- around 1070-1320 ---\n'
sed -n '1070,1320p' crates/perry-runtime/src/thread.rs
# Find js_throw and the spawn_impl comment mentioned in the review.
rg -n "js_throw|spawn_impl|longjmp|pinned promise|unsupported_transfer|guard_transferable" crates/perry-runtime/src/thread.rs crates -SRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("crates/perry-runtime/src/thread.rs")
lines = path.read_text().splitlines()
for start, end in [(846, 876), (966, 978), (1088, 1110), (1284, 1304)]:
print(f"\n--- {start}-{end} ---")
for i in range(start, end + 1):
print(f"{i:4}: {lines[i-1]}")
PYRepository: PerryTS/perry
Length of output: 4779
Drop the temporary Vecs before guard_transferable can throw.
js_throw longjmps, so serialized_elements, caps, and all_results stay on the heap on this path. Repeated caught TypeErrors can leak memory unboundedly. The call sites need to release those buffers before throwing, or restructure the check so it runs without allocating them first.
🤖 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 `@crates/perry-runtime/src/thread.rs` around lines 466 - 480, The
`guard_transferable` checks in `thread.rs` can throw via `js_throw`, which
longjmps and leaves heap-backed buffers like `serialized_elements`, `caps`, and
`all_results` unreleased on the error path. Update the
`spawn`/`parallelMap`/`parallelFilter` call sites to run `guard_transferable`
before allocating those temporary Vecs, or explicitly drop/free the buffers
before any possible throw. Use the existing `throw_unsupported_transfer` and
`guard_transferable` helpers to keep the validation on the calling thread
without retaining allocated state.
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
1 participant
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.
Three mechanical cross-thread GC-safety wins identified by the 2026-07-09 GC audit (§6, threading; #6185). These reduce silent corruption to loud failure / eliminate a leak without committing to the larger single-JS-thread-vs-multi-heap threading-model decision (still open — see "What remains").
Perry is single-JS-thread by default;
perry/threadgivesspawn()/parallelMap/parallelFilterper-thread arenas + GC, values cross bySerializedValuedeep-copy, and results return via the globalPENDING_THREAD_RESULTSdrained in the microtask pump.Shipped
1. Loud
SerializedValuefailures (audit P2 → not-silent)serialize_nanbox_for_threadsilently lowered every non-transferable runtime type — Map, Set, Promise, Error, TypedArray, Buffer, Symbol (POINTER_TAG'd butGC_TYPE_STRING), Temporal, native handles, unmaterialized lazy JSON arrays — toInline(TAG_UNDEFINED). A captured or returned value of one of these crossed aperry/threadboundary asundefinedwith no diagnostic, contradicting the documented deep-copy contract.Now the serializer emits a named
SerializedValue::Unsupported(&'static str)marker, and the transfer boundary raises a catchableTypeError("Cannot transfer a Map across a perry/thread boundary (unsupported type)"):spawn,parallelMap,parallelFilter):guard_transferablethrows synchronously on the calling/main thread, right after serialization.spawnRETURN path: the worker cannot throw (it has nosetjmpframe), so the marker rides back in the serialized result andjs_thread_process_pendingrejects the returned promise on the main thread — observable viaawait/.catch.The recursive
first_unsupported_transfer_typewalks arrays / object fields / closure captures, so a Map nested inside a returned object is caught too.Preserved transfers: every arm that previously worked is untouched — arrays, plain objects, closures, strings, numbers, int32, BigInt, Date,
SharedArrayBuffer, and all inline tags. Only the arms that previously became silentundefinednow throw, so no working transfer regresses.Safety argument (throw placement + leak avoidance):
js_throwlongjmps and does not run Rust destructors. Inspawn_implthe captures are therefore serialized and guarded before the cross-thread promise is allocated + pinned, so a throw cannot leak a pinned promise. Throwing only ever happens on the calling/main thread (which has an activesetjmpframe from compiled JS); worker-side failures are surfaced by promise rejection instead of a throw with no target.2.
Drop for MallocState— free worker malloc objects at thread exit (audit P1 leak)thread_local! MALLOC_STATEtracked a worker's malloc-heap objects (promises / maps / errors / large closures / tracked buffers) inobjects: Vec<*mut GcHeader>with noDrop. Aspawn/parallelMapworker leaked all of them at exit — and because the first malloc-count GC needs 100k objects, per-request workers effectively never collected once. Added aDropthat reclaims every tracked block via the samedealloc(Layout)pathprocess_sweep_header/sweep_malloc_objectsuse for one object.Drop-ordering safety argument:
MALLOC_STATEisthread_local!, soDropruns only at thread teardown, never mid-program.SerializedValuedeep-copy — no other thread holds a raw pointer into this heap — so reclaiming these blocks cannot dangle a reference anywhere else.MALLOC_STATEis not dropped until process teardown, so this never yanks live objects from running code.deallocwithgc_type_finalize_unmarked_payload/layout_clear_for_ptr, which touch other thread-locals (MAP_REGISTRY,MAP_INDEX,PROMISE_CONTEXTS, async-hooks queues). TLS destruction order is unspecified, so reaching an already-destroyed TLS during thisDropwould panic ("cannot access a Thread Local Storage value during or after destruction") and abort the thread — worse than the leak. We reclaim only the object blocks (the audit's primary worker-exit leak); external side-allocations (a Map's entry table, an error's side tables) are left to the fuller rework, and the re-entrantMALLOC_STATE.with(...)the sweep bookkeeping performs is avoided. Pinned objects are skipped, mirroring the sweep, so a cross-thread promise pinned for an in-flight result is never yanked.Scoped to
MALLOC_STATEper the task's "only if clearly safe" guidance; the small-buffer slab / buffer registry are not touched here (their finalizer-free freeing safety wasn't clear).Deferred
3. Owner-ThreadId-tagged timer dispatch (audit P0 mitigation) — BLOCKED
The naive slice (tag each queued timer with
std::thread::current().id()at schedule time; have the tick/fire functions skip entries whose owner!=current thread) would break all Android timer delivery, and this is a real, active configuration — exactly the "timers legitimately fire on a different thread than created" case flagged in the task:crates/perry-ui-android/src/app.rsnativePumpTick(JNI, ~8ms) callsjs_callback_timer_tick/js_interval_timer_tickon the UI thread, while TypeScript (and thussetTimeout/setIntervalscheduling) runs on the perry-native thread. The in-file comment states this explicitly: "Timer state is global (Mutex-protected), so this works from the UI thread even though timers were registered on the perry-native thread."timer.rs's own module doc documents the same design: "Uses global Mutex-protected state (not thread_local) so that timers registered on one thread can be pumped from another. This is critical on Android…"A blanket owner-skip would make the UI-thread pump skip every perry-native-registered timer →
setTimeout/setIntervalnever fire on Android. A safe variant (quarantine only knownspawn/parallelMapworker-origin timers) is no longer the "narrow mechanical slice" this PR targets — it needs a worker-thread registry and answers to "should a quarantined timer ever fire". Per the audit this P0 belongs to the per-thread-queue rework (Wave 4 threading-model decision), not this slice. No timer code changed.Tests
PERRY_NO_AUTO_OPTIMIZE=1 cargo test -p perry-runtime --lib -- --test-threads=1→ 1219 passed, 0 failed. New unit tests:serialize_nanbox_for_threadon a real Map yieldsUnsupported("Map")(the concrete audit case, not silent undefined); the boundary check recurses through array/object/closure trees; supported values (undefined/null/bool/int32/f64/string/array + round-trip) still transfer with noUnsupported; type-name mapping is human-readable.Dropcan't be exercised directly from a unit test (fires only at real thread teardown), so a#[cfg(test)]helper drives the shared free path on a TLS-independentMallocState: asserts the exact freed-byte total, thatobjectsis drained (idempotent second call frees 0), and that pinned objects are skipped.What remains for the full threading-model decision
The audit's Wave 4: either enforce single-JS-thread discipline mechanically (owner-tagged per-thread queues + compiler rejection of module-global access in thread closures) or commit to real multi-heap isolation. Beyond deferred #3, still open from §6: module globals aliasing the main heap into workers, per-thread GCs scanning shared global tables without coordination, the
await-loop draining process-global completion queues cross-heap, andws.rs's deferred-resolution tag bug.Ref #6185; 2026-07-09 GC audit §6.
Summary by CodeRabbit
undefined.parallelMap,parallelFilter, and background task results now surface transfer issues more reliably.