gc: fix collection fallbacks, shared actions, and heap dumps by youknowone · Pull Request #1160 · youknowone/pyre

youknowone · 2026-08-11T15:33:24Z

Follow-up fixes discovered after #1122 merged.

Summary

report a completed MARKING → SCANNING transition when the cranelift or wasm collect_step trampoline has no active collector
make gc_isenabled() return the enabled-by-default result before the process-global collector is initialized
keep space.actionflag, space.user_del_action, and the GIL release action process-owned instead of duplicating them per execution context
dispatch a worker-triggered GC hook on the collecting worker, matching PyPy
root the shared finalizer and GIL action fields exactly once as process-global roots
route sandbox heap-dump writes through host_seam::ops::write while preserving the native raw-write fallback
allocate fresh _json runtime strings—encode_basestring* results, decoded scanstring values, JSON error notes, float-key coercions, and one-shot encoder chunks—in the managed heap instead of leaking them as host-allocated immortal strings
allocate explicit types.GenericAlias.__repr__ results in the managed heap, matching PyPy's f-string result
allocate dynamic ContextVar and Token repr results in the managed heap instead of leaking each rendered string
allocate dynamic structseq repr results in the managed heap, matching PyPy's ordinary formatted-string allocation
allocate array.array.tounicode() results in the managed heap instead of leaking each converted Unicode buffer
allocate explicit array.array.__repr__ results in the managed heap for both numeric and Unicode arrays
allocate explicit collections.deque.__repr__ results in the managed heap, including maxlen-formatted results
allocate explicit re.Pattern.__repr__ results in the managed heap instead of leaking each formatted pattern string
allocate explicit re.Match.__repr__ results in the managed heap instead of leaking each formatted match string
preserve weak-proxy str() result identity and managed lifetime by forwarding through the object-space string operation
allocate unpickled SHORT_BINUNICODE, BINUNICODE, and BINUNICODE8 results in the managed heap
allocate non-ASCII unicodedata.normalize() results in the managed heap and preserve PyPy's O(1) ASCII identity fast path
allocate explicit types.SimpleNamespace.__repr__ results and recursive guard strings in the managed heap
allocate re.sub, re.subn, and Match.expand Unicode results in the managed heap
allocate _sre group, findall, split, and subclass-normalization slices in the managed heap with relocation-safe container assembly
allocate native time.strftime() results in the managed heap on both Unix and Windows
allocate time.asctime() and native time.ctime() results in the managed heap through the shared upstream-style formatter
allocate explicit os.DirEntry.__repr__ results in the managed heap, matching PyPy's space.newtext result
allocate live and released explicit memoryview.__repr__ results in the managed heap across native and Wasm backends
allocate live and dead explicit weakref, weak proxy, and callable weak proxy repr results in the managed heap
allocate live and closed mmap.mmap.__repr__ results in the managed heap on native backends
remove the duplicate wrap_raw_nodes documentation summary

Root cause

PyPy stores its action flag and async GC/finalizer actions on the object space. Pyre stored the flag on each ExecutionContext, while hook actions retained the boot context flag. A worker gc.collect() therefore fired a bit that the worker dispatch loop never read; the callback waited until the boot thread reached an opcode and ran on the wrong thread. The shared action state now follows the upstream object-space ownership shape.

The generic w_str_from_wtf8 constructor is an off-GC bootstrap/structural-string path. _json used it for fresh runtime results from encode_basestring*, scanstring, dynamically generated error notes, float-key coercions, and one-shot encoder chunks, so those strings and their WTF-8 buffers were immortal. They now use w_str_from_wtf8_managed; values that cross a subsequent tuple/list allocation are pinned on the shadow stack first. The PyPy oracle and the fixture verify that each observable result identity appears in gc.get_objects(). The explicit types.GenericAlias.__repr__ descriptor had the same defect even though ordinary repr(alias) already used a managed display path; its direct result now follows PyPy's ordinary f-string allocation. ContextVar.__repr__ and Token.__repr__ likewise returned freshly rendered values through the bootstrap constructor; both now use the managed path verified by the PyPy identity oracle. Structseq reprs had the same runtime-allocation defect; sys.version_info now verifies the shared repr path on native and Wasm without relying on an OS module. array.array.tounicode() also returned a fresh conversion through the bootstrap constructor, including as an intermediate in Unicode-array repr; it now follows PyPy's space.newutf8 managed allocation. The explicit array.array.__repr__ descriptor independently returned its final formatted string through the bootstrap constructor; its numeric and Unicode results now match PyPy's managed space.newtext result. The explicit collections.deque.__repr__ descriptor had the same final-result leak even though ordinary repr(deque) already passed through the managed display path; it now matches PyPy's %-formatted managed result. The explicit re.Pattern.__repr__ descriptor likewise used the bootstrap constructor while PyPy returns space.newtext; its direct descriptor result is now managed. The explicit re.Match.__repr__ descriptor had the identical bootstrap-constructor defect and now also follows PyPy's managed space.newtext result. Weak-proxy __str__ diverged more deeply: PyPy forwards to space.str(w_obj) and returns the referent's managed result object unchanged, while Pyre flattened it to WTF-8 and rebuilt an immortal copy. The proxy now forwards through the object-space-equivalent builtin_str, preserving both identity and GC lifetime. _pickle's shared UTF-8 loader similarly rebuilt SHORT_BINUNICODE, BINUNICODE, and BINUNICODE8 values through the bootstrap constructor, while PyPy uses space.newtext for all three; unpickled Unicode values now use the managed constructor. unicodedata.normalize() also rebuilt every result through the bootstrap constructor. Non-ASCII normalization now returns managed base strings, while exact ASCII strings follow PyPy's space.newutf8 buffer-identity behavior through an O(1) return; string subclasses still become managed base strings. types.SimpleNamespace.__repr__ had the same final-result defect in both its ordinary formatted result and recursive namespace(...) guard; both paths now use managed constructors, matching PyPy's app-level formatting behavior. _sre's shared substitution-output builder also returned Unicode sub, subn, and Match.expand results through the bootstrap constructor, while PyPy uses space.newutf8; the shared result now uses the managed constructor, and subn reloads it from a shadow-stack slot across tuple allocation. _sre::slice_subject had the same bootstrap-allocation defect across Match.group/__getitem__/groups/groupdict, findall, split, and the no-match subclass-normalization path. These slices now use PyPy's space.newutf8-equivalent managed constructor. Tuple/dict assembly reloads translated live GCREFs from shadow-stack slots, while findall and split now accumulate directly into a rooted managed list like PyPy instead of retaining every result in an off-heap Rust Vec, avoiding an O(number of matches) shadow stack.

time.strftime() had the same runtime-string defect in both native implementations: PyPy returns space.newutf8(decoded, size), but Pyre's Unix and Windows valid-result branches used the bootstrap constructor. Both now return managed strings; the locale surrogateescape fallback was already managed.

time.asctime() and time.ctime() likewise returned freshly formatted values through w_str_new. PyPy routes both through _asctime and ordinary % formatting. Pyre now allocates the shared formatted result as managed and removes the separate Windows _ctime64 branch so native ctime() follows the same localtime → _asctime shape on Unix and Windows.

The explicit os.DirEntry.__repr__ descriptor also assembled a fresh <DirEntry ...> value through the bootstrap WTF-8 constructor, while PyPy returns space.newtext; its direct result is now managed, matching the already-managed ordinary repr(entry) display path.

The explicit memoryview.__repr__ descriptor formatted both live <memory at ...> and released <released memory at ...> values through w_str_new, while PyPy's shared W_Root.getrepr returns space.newtext. Both labels now use the managed constructor, matching the already-managed ordinary repr(view) path on native and Wasm.

The shared W_WeakrefBase.descr__repr__ path for ReferenceType, ProxyType, and CallableProxyType likewise returned live and dead labels through w_str_new, while PyPy delegates all of them to W_Root.getrepr and space.newtext. The shared result now uses the managed constructor, covering both explicit descriptor calls and ordinary repr() for all three weak-reference kinds in both states.

W_MMap.descr_repr had the same defect in both branches: live mappings formatted their access, length, position, and offset through w_str_new, while closed mappings returned a raw structural string. PyPy returns space.newtext after % formatting in both cases, so both native paths now allocate managed results; the oracle fixture verifies explicit and ordinary repr identities before and after close().

Validation

PyPy oracle: pypy3 pyre/bench/synth/gc_hook_worker_thread.py
PyPy oracle: PYPYLOG=jit-summary:- pypy3 pyre/bench/synth/gc_json_string_collectable.py
cargo fmt --all --check
cargo check --features dynasm
cargo test --features dynasm
cargo check -p pyrex --bin pyre --features sandbox
python3 pyre/check.py --backend dynasm,cranelift --no-synthetic --no-cpython-suite — dynasm 17/17, cranelift 17/17
python3 pyre/check.py --backend dynasm,cranelift,wasm --synthetic-only --synthetic-pattern gc_*.py --no-cpython-suite — dynasm 28/28, cranelift 28/28, wasm 22/22

Summary by CodeRabbit

Bug Fixes
- Improved garbage-collection state transitions across native and WebAssembly builds.
- Fixed heap-dump writing across platforms, including sandbox support, error handling, and short-write detection.
- Corrected garbage-collection status reporting before initialization.
- Improved tracking of finalizers and thread-related garbage-collection roots.
- Fixed JSON, context-variable, and generic-alias strings so generated values are properly managed and collectible.
Tests
- Added coverage for worker-thread garbage collection and runtime string collectability.
- Added JIT statistics coverage for compilation, guards, loops, and retracing.

coderabbitai · 2026-08-11T15:33:34Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

The PR corrects fallback GC transitions, adds sandbox-aware heap-dump writing, and moves finalizer and GIL actions to process-wide storage. It also adds managed string construction, collectability probes, worker-thread GC-hook coverage, and JIT statistics fixtures.

Changes

Shared interpreter actions

Layer / File(s)	Summary
Shared action state `pyre/pyre-interpreter/src/executioncontext.rs`, `pyre/pyre-interpreter/src/module/signal/interp_signal.rs`, `pyre/pyre-interpreter/src/module/gc/hook.rs`	`ExecutionContext` now uses shared `SpaceActionFlag` state. Finalizer and signal actions resolve through process-wide storage.
Global action root walking `pyre/pyre-interpreter/src/eval.rs`, `pyre/pyre-interpreter/src/module/thread/mod.rs`	Root walking traces process-wide user-del and GIL action references.
Worker GIL registration `pyre/pyre-interpreter/src/module/thread/gil.rs`, `pyre/pyre-interpreter/src/module/thread/mod.rs`, `pyre/bench/synth/gc_hook_worker_thread.py`, `pyre/bench/synth/gc_hook_worker_thread.*.jitstats`	GIL registration uses `OnceLock`. Worker cleanup no longer shuts down per-context GIL actions. The fixture checks worker-thread GC-hook execution.

Heap-dump write integration

Layer / File(s)	Summary
Heap-dump write dispatch `majit/majit-gc/src/collector.rs`, `majit/majit-gc/src/lib.rs`	The GC crate exports `HEAP_DUMP_EIO` and supports optional host write callbacks. Heap-dump flushing handles native writes, errors, short writes, and buffer clearing.
Sandbox heap-dump adapter `pyre/pyre-interpreter/src/module/gc/mod.rs`	Sandbox builds translate host write errors and register the heap-dump writer before dumping.

GC state correction

Layer / File(s)	Summary
Fallback GC transitions `majit/majit-backend-cranelift/src/compiler.rs`, `majit/majit-backend-wasm/src/lib.rs`	Fallback transitions now use `MARKING` as the old state and `SCANNING` as the new state.

Managed string construction and fixtures

Layer / File(s)	Summary
Managed string paths `pyre/pyre-interpreter/src/module/_json/mod.rs`, `pyre/pyre-interpreter/src/_pypy_generic_alias.rs`, `pyre/pyre-interpreter/src/module/_contextvars/mod.rs`	JSON, generic-alias, and context-variable string paths now use managed WTF-8 constructors. JSON scan results and encoded strings are rooted before use.
Collectability probes and statistics `pyre/bench/synth/gc_json_string_collectable`, `pyre/bench/synth/gc_contextvar_repr_collectable`, `pyre/bench/synth/gc_generic_alias_repr_collectable*`	New probes verify GC visibility for generated strings. JIT statistics fixtures contain zero-initialized counters.

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

Sequence Diagram(s)

sequenceDiagram
  participant WorkerExecutionContext
  participant SpaceActionFlag
  participant GilInitialize
  participant ThreadRootWalker
  WorkerExecutionContext->>SpaceActionFlag: access shared action flag
  WorkerExecutionContext->>GilInitialize: register GIL action once
  GilInitialize->>ThreadRootWalker: expose retained action root
  ThreadRootWalker-->>WorkerExecutionContext: trace action reference

Possibly related issues

Remaining engine work: object-model/GC convergence, JIT trace correctness, and RPython toolchain bridge retirement #205: The process-wide GC roots, finalizer handling, and heap-dump write hook advance the issue’s GC infrastructure objectives.

Possibly related PRs

youknowone/pyre#784: Modifies related ExecutionContext, thread, and GC root handling.
youknowone/pyre#986: Uses related GIL and global-action infrastructure.
youknowone/pyre#1122: Corrects related fallback GC transition state reporting.

Poem

A rabbit watched shared roots align,
Heap bytes crossed the sandbox line.
MARKING changed to SCANNING bright,
Worker hooks ran on the worker right.
Managed strings stayed in sight.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title concisely summarizes the main GC changes, including collection fallbacks, shared actions, and heap-dump handling.

✨ 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 gc-decouple

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

chatgpt-codex-connector

💡 Codex Review

https://github.com/youknowone/pyre/blob/32dc502ba2ebab122734b9ac23fb4239a40a6c0f/pyre-interpreter/src/executioncontext.rs#L1967-L1970
Keep signal dispatch on the signal-enabled thread

Sharing this ticker means any worker can now enter the shared periodic-action dispatcher when a signal makes it negative. However, CheckSignalAction::poll_for_signals_unlocked still unconditionally calls report_signal(ec, n) under its obsolete “pyre is single-threaded” assumption (module/signal/interp_signal.rs:316-339). Thus, if a worker reaches an opcode before the main interpreter thread after SIGINT or another handled signal, the Python handler—and commonly KeyboardInterrupt—runs on that worker. PyPy's interp_signal.py:128-139 instead checks threadlocals.signals_enabled() and defers via fire_in_another_thread until the main thread resumes; that thread-sensitive path must be ported as part of making the action flag shared.

AGENTS.md reference: AGENTS.md:L231-L232

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

github-actions · 2026-08-11T17:26:07Z

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit db5be09).
Updated: 2026-08-12T10:18:15.371Z

Files in the reviewed diff

majit/majit-backend-cranelift/src/compiler.rs
majit/majit-backend-wasm/src/lib.rs
majit/majit-gc/src/collector.rs
majit/majit-gc/src/lib.rs
pyre/bench/synth/gc_hook_worker_thread.py
pyre/bench/synth/gc_native_strings_collectable.py
pyre/bench/synth/gc_runtime_strings_collectable.py
pyre/pyre-interpreter/src/_pypy_generic_alias.rs
pyre/pyre-interpreter/src/_structseq.rs
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/executioncontext.rs
pyre/pyre-interpreter/src/function.rs
pyre/pyre-interpreter/src/module/_collections/mod.rs
pyre/pyre-interpreter/src/module/_contextvars/mod.rs
pyre/pyre-interpreter/src/module/_json/mod.rs
pyre/pyre-interpreter/src/module/_pickle/mod.rs
pyre/pyre-interpreter/src/module/_sre/interp_sre.rs
pyre/pyre-interpreter/src/module/_weakref/interp__weakref.rs
pyre/pyre-interpreter/src/module/array/mod.rs
pyre/pyre-interpreter/src/module/gc/hook.rs
pyre/pyre-interpreter/src/module/gc/mod.rs
pyre/pyre-interpreter/src/module/mmap/interp_mmap.rs
pyre/pyre-interpreter/src/module/posix/interp_posix.rs
pyre/pyre-interpreter/src/module/signal/interp_signal.rs
pyre/pyre-interpreter/src/module/sys/vm.rs
pyre/pyre-interpreter/src/module/thread/gil.rs
pyre/pyre-interpreter/src/module/thread/mod.rs
pyre/pyre-interpreter/src/module/time/interp_time.rs
pyre/pyre-interpreter/src/module/unicodedata/mod.rs
pyre/pyre-interpreter/src/typedef.rs

1. Regressions to PyPy parity introduced by this patch

pyre/pyre-interpreter/src/module/unicodedata/mod.rs:279 ↔ pypy/module/unicodedata/interp_ucd.py:178: the new ASCII fast path returns args[1] unchanged, but PyPy returns space.newutf8(content, strlen), a fresh base W_UnicodeObject. Thus unicodedata.normalize(form, exact_ascii_str) is exact_ascii_str becomes true in pyre and false in PyPy. upstream/main allocated a fresh result, so this regresses parity.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

pyre/pyre-interpreter/src/typedef.rs:26686 ↔ pypy/interpreter/generator.py:48: async_generator_descr_repr still uses immortal w_str_new, while PyPy returns space.newtext(...). The neighboring generator/coroutine paths were converted to managed strings by this patch, but this existing async-generator result remains non-collectable.

4. Structural adaptations

pyre/pyre-interpreter/src/executioncontext.rs:1967 ↔ pypy/interpreter/baseobjspace.py:450: OnceLock<usize> plus SpaceActionFlag represents PyPy’s ObjSpace.actionflag. This is a Rust ownership/typed-object-space adaptation, preserving one shared action flag per runtime rather than storing it in each execution context.
pyre/pyre-interpreter/src/executioncontext.rs:229 ↔ pypy/interpreter/executioncontext.py:724: the process-owned UserDelAction and explicit root walker replace PyPy’s GC-visible space.user_del_action field. This is required because the Rust allocation is outside PyPy’s translated object graph.
majit/majit-gc/src/collector.rs:101 ↔ rpython/memory/gc/inspector.py:216: sandboxed heap dumping routes the raw write through a host seam instead of calling raw_os_write directly. This is a sandbox/host-I/O adaptation; short-write and errno behavior remain aligned.

coderabbitai

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 `@pyre/bench/synth/gc_hook_worker_thread.py`:
- Line 16: Rename the unused parameter in the on_collect callback from stats to
_stats, preserving the callback’s signature and behavior.

🪄 Autofix

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: ASSERTIVE

Plan: Pro Plus

Run ID: f7083db2-f106-4b00-a5be-a48a78684ca9

📥 Commits

Reviewing files that changed from the base of the PR and between d6b6547 and 32dc502.

📒 Files selected for processing (14)

majit/majit-backend-cranelift/src/compiler.rs
majit/majit-backend-wasm/src/lib.rs
majit/majit-gc/src/collector.rs
majit/majit-gc/src/lib.rs
pyre/bench/synth/gc_hook_worker_thread.cranelift.jitstats
pyre/bench/synth/gc_hook_worker_thread.dynasm.jitstats
pyre/bench/synth/gc_hook_worker_thread.py
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/executioncontext.rs
pyre/pyre-interpreter/src/module/gc/hook.rs
pyre/pyre-interpreter/src/module/gc/mod.rs
pyre/pyre-interpreter/src/module/signal/interp_signal.rs
pyre/pyre-interpreter/src/module/thread/gil.rs
pyre/pyre-interpreter/src/module/thread/mod.rs

coderabbitai · 2026-08-11T17:28:07Z

+done.acquire()
+
+
+def on_collect(stats):


📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Rename the unused callback parameter.

Ruff reports stats as unused. Rename it to _stats and preserve the callback signature.

Proposed fix

-def on_collect(stats): +def on_collect(_stats):

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

def on_collect(stats):

def on_collect(_stats):

🧰 Tools

🪛 Ruff (0.16.1)

[warning] 16-16: Unused function argument: stats

(ARG001)

🤖 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 `@pyre/bench/synth/gc_hook_worker_thread.py` at line 16, Rename the unused parameter in the on_collect callback from stats to _stats, preserving the callback’s signature and behavior.

Source: Linters/SAST tools

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ebeb5beefd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-11T17:57:25Z


+#[cfg(feature = "sandbox")]
+fn heap_dump_write_via_host(fd: i32, bytes: &[u8]) -> Result<isize, i32> {
+    crate::host_seam::ops::write(fd, bytes)


Avoid releasing the GIL while the heap dump owns STW

In a sandbox build with more than one registered mutator, this call deadlocks: do_dump_rpy_heap holds a StwGuard (collector.rs:3538-3543), while the sandbox ops::write wrapper enters before_external_block (host_seam.rs:457-460). When that blocking guard is dropped, it waits for stw_requested to become false (gc_sync.rs:334-338), but this thread cannot clear the request until the write returns and its own StwGuard is dropped. Consequently, gc.dump_rpy_heap hangs whenever another runtime thread remains registered; this path needs a host write variant that does not leave the GIL/RUNNING census while the caller owns STW.

Useful? React with 👍 / 👎.

chatgpt-codex-connector

💡 Codex Review

https://github.com/youknowone/pyre/blob/49739b06e757d831da145796d44e04023e77c792/pyre-interpreter/src/module/signal/interp_signal.rs#L432
Keep signal polling on the main thread

When a signal arrives while a worker owns the GIL, registering CheckSignalAction on the shared flag lets that worker run poll_for_signals_unlocked first. Unlike upstream's pypy/module/signal/interp_signal.py:125-140, this implementation has no signals_enabled() gate; moreover, HANDLERS is thread-local (interp_signal.rs:80-109), so the worker consumes the process-wide pending signal, finds no handler in its empty table, and returns before the main thread can deliver it. This can silently swallow SIGINT or user-installed handlers in threaded programs; retain the shared action but defer signal consumption to the main thread as upstream does.

AGENTS.md reference: AGENTS.md:L148-L155

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1cf178703b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-11T18:34:17Z

    let action: &'static mut CheckSignalAction = Box::leak(CheckSignalAction::new(ec.space));
    let async_ptr: *mut dyn AsyncActionOps = &mut *action;
-    action.register_periodic_action(&mut ec.actionflag, false);
+    action.register_periodic_action(ec.actionflag.shared_mut(), false);


Defer signal polling from worker threads

With multiple Python threads, registering CheckSignalAction on the newly shared flag lets whichever worker next dispatches the ticker run poll_for_signals; the existing implementation at interp_signal.rs:316-340 unconditionally consumes the pending signal and calls its Python handler with that worker's execution context. Before this change, only the boot context's flag contained this action. Upstream pypy/module/signal/interp_signal.py:128-140 instead checks signals_enabled() and defers delivery until switching to the main thread, so this shared registration needs the same gate/handoff or a pending SIGINT can raise KeyboardInterrupt in a worker while the main thread continues.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

coderabbitai

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 `@pyre/bench/synth/gc_json_string_collectable.py`:
- Around line 22-27: Keep the assertion in the JSON error-note test tied
exclusively to the note produced by json.dumps/add_json_note: when the runtime
supports JSON context notes, fail if notes is empty instead of calling
exc.add_note. If older PyPy compatibility is required, isolate it behind an
explicit runtime capability check and ensure the GC ownership assertion still
validates the JSON-created note rather than the synthetic fallback.

🪄 Autofix

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: ASSERTIVE

Plan: Pro Plus

Run ID: 07577829-8aca-42d5-819b-60aef336c6c5

📥 Commits

Reviewing files that changed from the base of the PR and between ebeb5be and 1cf1787.

📒 Files selected for processing (2)

pyre/bench/synth/gc_json_string_collectable.py
pyre/pyre-interpreter/src/module/_json/mod.rs

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5788712637

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-11T18:55:57Z


 impl PeriodicAsyncActionOps for GilReleaseAction {}

+static GIL_RELEASE_ACTION: OnceLock<usize> = OnceLock::new();


Prevent mutable aliasing of the shared GIL action

When two Python threads are runnable and the periodic ticker fires, action_dispatcher invokes this singleton through &mut dyn AsyncActionOps; perform then calls yield_thread(), leaving that mutable borrow live while another thread acquires the GIL and can dispatch the same singleton through a second &mut. Shared GC actions have the same problem when their Python callbacks release the GIL. This is undefined behavior and can corrupt action state, so keep the action process-owned but redesign dispatch so no exclusive Rust borrow of a shared action spans a GIL handoff or app-level call.

AGENTS.md reference: AGENTS.md:L148-L155

Useful? React with 👍 / 👎.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1d627839c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-11T20:49:55Z

    let action: &'static mut CheckSignalAction = Box::leak(CheckSignalAction::new(ec.space));
    let async_ptr: *mut dyn AsyncActionOps = &mut *action;
-    action.register_periodic_action(&mut ec.actionflag, false);
+    action.register_periodic_action(ec.actionflag.shared_mut(), false);


Reuse the process-owned signal action

When a process creates another top-level execution context, such as the -i path that runs a script and then enters run_repl, the new EC has check_signal_action == None, so this allocates and registers another leaked CheckSignalAction. Because the action flag is now process-owned, the previous action remains in its periodic list; every ticker subsequently dispatches both, and embeddings that invoke the runner repeatedly grow this list without bound. Store/reuse the signal action with the shared object-space state rather than guarding installation on each EC.

AGENTS.md reference: AGENTS.md:L148-L155

Useful? React with 👍 / 👎.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c544ce3ebd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-11T21:53:39Z

+    fn decrement_ticker(&mut self, by: isize) -> isize {
+        self.inner_mut().decrement_ticker(by)


Use synchronized access for the shared signal ticker

With multiple Python threads, this newly shared delegation makes whichever GIL-owning worker is executing bytecode read and modify the same plain isize ticker that the OS signal handler writes through signalstate::rearm_ticker using ptr::write_volatile. The GIL does not serialize an asynchronous handler running on another OS thread, and volatile access is not atomic in Rust, so a signal arriving while a worker calls decrement_ticker creates a data race (undefined behavior, with possible missed or corrupted ticker updates). The process-owned ticker needs signal-safe synchronized storage/access rather than ordinary isize reads and writes.

Useful? React with 👍 / 👎.

…_step trampolines Both are installed into ACTIVE_COLLECT_STEP, so they stood in front of the already-corrected majit-gc fallback and the trait default. Assisted-by: Claude

gc_query_reentrant reaches singleton_ref, which panics when store_singleton has not run; would_collect asks this from the interpreter allocation path. Assisted-by: Claude

Assisted-by: Claude

HeapDumpWriter reached the descriptor with a raw libc::write. Under --features sandbox that descriptor is the guest's, so gc._dump_rpy_heap(1) put dump bytes on the marshalling pipe and a virtualized descriptor named an unrelated host fd. The write now goes through a process-global hook that the sandbox build fills in with host_seam::ops::write; with no hook installed the native path is unchanged. HEAP_DUMP_EIO is exported so both sides name one constant, and its comment now states pyre's own reason for a fixed code rather than attributing it to inspector.py, which raises the real errno (inspector.py:212-223). Assisted-by: Claude

PyPy owns the action flag, user-finalizer action, and GIL release action on the object space. Pyre instead gave each execution context a separate flag, so a worker collection fired a GC hook bit that only the boot context dispatched.\n\nKeep one process-owned flag and action instance, root their managed fields once, and add a worker-thread oracle fixture that verifies the hook runs on the collecting thread.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1195dfdcbe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-12T05:41:51Z

    for &w_arg in group_args {
-        let span = do_span(m, Some(w_arg))?;
-        results.push(unsafe { slice_w(m, span, w_none()) });
+        let span = do_span(m.get() as *const W_SRE_Match, Some(w_arg))?;


Pin all group selectors before allocating slices

When Match.group() receives multiple selectors, the first slice_w now creates a managed string and can trigger a moving collection, but the remaining selectors are retained only as raw pointers in group_args. The builtin gateway's native argument copy is not forwarded during that collection, so a call such as m.group(dynamic_name1, dynamic_name2) can pass a stale second name to do_span, producing incorrect lookup or memory corruption when the nursery fills. Pin every selector before the loop and reload it from the shadow stack, as is already done for m and the results.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

youknowone changed the title ~~gc: fix collection fallbacks and sandbox heap dumps~~ gc: fix collection fallbacks, shared actions, and heap dumps Aug 11, 2026

youknowone marked this pull request as ready for review August 11, 2026 17:20

chatgpt-codex-connector Bot reviewed Aug 11, 2026

View reviewed changes

coderabbitai Bot reviewed Aug 11, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 11, 2026

View reviewed changes

coderabbitai Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread pyre/bench/synth/gc_json_string_collectable.py Outdated

chatgpt-codex-connector Bot reviewed Aug 11, 2026

View reviewed changes

youknowone force-pushed the gc-decouple branch from 53efead to 6719b46 Compare August 12, 2026 05:07

youknowone added 16 commits August 12, 2026 14:08

gc: report a completed transition from the cranelift and wasm collect…

04932b4

…_step trampolines Both are installed into ACTIVE_COLLECT_STEP, so they stood in front of the already-corrected majit-gc fallback and the trait default. Assisted-by: Claude

gc: answer gc_isenabled before the GC singleton is stored

46447ed

gc_query_reentrant reaches singleton_ref, which panics when store_singleton has not run; would_collect asks this from the interpreter allocation path. Assisted-by: Claude

gc: drop the repeated summary line from wrap_raw_nodes' doc

5e2c2f2

Assisted-by: Claude

gc: collect JSON encoder strings

96f65d7

gc: collect JSON decoder strings

0b0d65a

gc: collect JSON error notes

b676483

gc: collect JSON encoder chunks

7978c7c

gc: collect JSON float keys

59aeffd

gc: collect explicit GenericAlias reprs

99d5d1d

gc: collect ContextVar reprs

8b9ca39

gc: collect ContextVar token reprs

ffbccee

gc: collect structseq reprs

b390e6e

gc: collect array unicode strings

f81d30f

gc: collect explicit array reprs

842a4d5

youknowone added 15 commits August 12, 2026 14:08

gc: collect asctime and ctime results

3290819

gc: collect DirEntry reprs

9e98949

gc: collect memoryview reprs

7c4c719

gc: collect weakref reprs

d82f02a

gc: collect mmap reprs

eaf62b0

gc: collect FileIO reprs

f310d67

gc: collect exception reprs

7e2dddf

gc: collect exception str results

4f967aa

gc: collect exception group render results

c4a7a4d

gc: collect bound method reprs

1f8aa4b

gc: collect super reprs

92cbe98

gc: collect union reprs

2348898

gc: collect function reprs

2ecd54f

gc: collect builtin function reprs

2d668fb

ci: skip mmap repr fixture when unavailable

1195dfd

youknowone force-pushed the gc-decouple branch from 6719b46 to 1195dfd Compare August 12, 2026 05:36

chatgpt-codex-connector Bot reviewed Aug 12, 2026

View reviewed changes

youknowone added 11 commits August 12, 2026 14:57

gc: collect getset descriptor reprs

dd5ef4e

gc: collect member descriptor reprs

088386b

gc: collect method descriptor reprs

70682ec

gc: collect classmethod descriptor reprs

e3f2e81

ci: match mmap skip output on Windows

044b249

gc: collect method-wrapper reprs

80e9453

gc: collect type reprs

f68c5cb

gc: collect cell reprs

fb260d6

gc: collect generator reprs

1408a4f

test: consolidate GC collectability fixtures

a3b84e4

gc: consolidate remaining repr collectability coverage

db5be09

youknowone merged commit f5e308b into main Aug 12, 2026
17 checks passed

youknowone deleted the gc-decouple branch August 12, 2026 13:58


		impl PeriodicAsyncActionOps for GilReleaseAction {}

		static GIL_RELEASE_ACTION: OnceLock<usize> = OnceLock::new();

		fn decrement_ticker(&mut self, by: isize) -> isize {
		self.inner_mut().decrement_ticker(by)

Conversation

youknowone commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Validation

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

github-actions Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Codex parity review

1. Regressions to PyPy parity introduced by this patch

2. Other mismatches introduced by this patch

3. Pre-existing mismatches (already present before this patch)

4. Structural adaptations

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

youknowone commented Aug 11, 2026 •

edited

Loading

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading

github-actions Bot commented Aug 11, 2026 •

edited

Loading