Skip to content

fix(gc): readline/stdin listener closures are moving-GC roots - #6858

Merged
proggeramlug merged 1 commit into
PerryTS:mainfrom
proggeramlug:readline-gc-root-fix
Jul 27, 2026
Merged

fix(gc): readline/stdin listener closures are moving-GC roots#6858
proggeramlug merged 1 commit into
PerryTS:mainfrom
proggeramlug:readline-gc-root-fix

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Problem

The stdin listener lists (DATA_CALLBACKS / KEYPRESS_CALLBACKS / READABLE_CALLBACKS — the process.stdin.on('data'/'keypress'/'readable') lists) and the readline interface callbacks (line / close / question) hold JS closure pointers as raw i64 in native storage that no GC scanner ever visited (the source even documented this: "No GC scanner ever visited these").

This is sound under the non-moving collector — the closures stay live via the JS listener graph and are never relocated. But under a moving collector (copying minor), the young closure is relocated and these native slots keep the stale from-space pointer, so the next js_readline_process_pending dispatches through the stale pointer → reads forwarding-pointer bytes → TypeError: value is not a function.

Fix

Register a stdlib:readline mutable root scanner (scan_readline_roots_mut) that visits + rewrites all those closure slots when the moving collector runs — mirroring the existing stdlib:events (EventEmitter) scanner and the net.Socket listener fix (#35). Registered at the three store sites (stdin_on_op, js_readline_question, js_readline_on).

Same class of latent bug as the EventEmitter/net.Socket cases; readline was simply missed. Found while validating a moving idle-reclaim; the primary-fault backtrace was js_readline_process_pending → dispatch → [stale bare-address callee pointing at a moved NurseryEden closure].

Testing

Validated end-to-end: with the moving idle-reclaim enabled, the reclaim went from crashing 3/3 runs to surviving 3/3. Dispatch clones each callback list and drops the lock before invoking, so no lock/borrow is held across an allocation that could re-enter the scanner. No behavior change under the default non-moving collector.

https://claude.ai/code/session_01GNQBccgAPBmz3txWnAFxpG

Summary by CodeRabbit

  • Bug Fixes
    • Improved readline and standard-input callback stability when memory cleanup occurs.
    • Ensured registered listeners, one-time callbacks, and interface events remain valid during garbage collection.

The stdin listener lists (DATA/KEYPRESS/READABLE_CALLBACKS) and readline
interface callbacks (line/close/question) held JS closure pointers as raw
i64 in native storage that no GC scanner ever visited. Sound under the
non-moving collector (closures stay live via the JS listener graph, never
relocated), but the copying minor RELOCATES a young closure and leaves these
native slots pointing at the stale from-space address -> js_readline_process_pending
dispatches through the stale pointer -> 'value is not a function'.

Register a mutable root scanner (stdlib:readline) so the moving collector
rewrites+marks these slots, mirroring stdlib:events (EventEmitter) and the
net.Socket listener fix (PerryTS#35). Found via the idle-reclaim campaign: the
primary fault backtrace was js_readline_process_pending -> a bare-address
callee pointing at a moved NurseryEden closure (obj_type=4).
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Readline now registers a moving-GC root scanner for stdin listeners, readline callbacks, per-interface callback state, and pending iteration state. Scanner initialization is guarded by Once and triggered by stdin listener, question, and on registration paths.

Changes

Readline GC root scanning

Layer / File(s) Summary
Scanner and registration wiring
crates/perry-stdlib/src/readline.rs
Adds one-time scanner registration, scans shared and thread-local readline callback roots, and initializes scanning from stdin and readline callback registration entry points.

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

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description has useful details but does not follow the required template and omits the Related issue and checklist sections. Restructure it into the repo template with Summary, Changes, Related issue, Test plan, Screenshots/output, and Checklist, and include a link to the tracking issue or n/a.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the readline/stdin moving-GC root fix.
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.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-stdlib/src/readline.rs`:
- Around line 197-206: Move the one-time GC scanner registration from the
`.question()`/`.on()` usage paths into `ReadlineInterfaceState::new()`, which is
used by `create_interface_from_options()` and `js_readline_create_interface()`.
Ensure `ensure_gc_scanner_registered()` runs during interface construction
before storing or exposing the NaN-boxed input/output values, while preserving
the existing `Once`-guarded registration behavior.
🪄 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: 4f82ceda-7c8f-4dc6-8943-e6a591dc1708

📥 Commits

Reviewing files that changed from the base of the PR and between 563c359 and 3568a6c.

📒 Files selected for processing (1)
  • crates/perry-stdlib/src/readline.rs

Comment on lines +197 to +206
static READLINE_GC_REGISTERED: std::sync::Once = std::sync::Once::new();

fn ensure_gc_scanner_registered() {
READLINE_GC_REGISTERED.call_once(|| {
perry_runtime::gc::gc_register_mutable_root_scanner_named(
"stdlib:readline",
scan_readline_roots_mut,
);
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate where ReadlineInterfaceState is constructed and readline interfaces
# are created, to confirm whether input/output are stored before any of the
# three ensure_gc_scanner_registered() call sites can run.
rg -n 'ReadlineInterfaceState\s*\{' crates/perry-stdlib/src/readline.rs -B15 -A5
rg -n 'fn js_readline_create' crates/perry-stdlib/src/readline.rs -A25
rg -n 'ensure_gc_scanner_registered' crates/perry-stdlib/src/readline.rs

Repository: PerryTS/perry

Length of output: 3171


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== readline.rs relevant sections =="
sed -n '70,135p' crates/perry-stdlib/src/readline.rs
sed -n '1120,1185p' crates/perry-stdlib/src/readline.rs

echo "== all ensure_gc_scanner_registered sites in repo =="
rg -n 'ensure_gc_scanner_registered\(\)' .

echo "== read/write/store field patterns for readline state =="
rg -n 'ReadlineInterfaceState|readline_create_interface|create_interface_from_options|input:|output:|\.input|\.output|question_callback|line_callback|listener_callbacks|with_interface|with_interface_mut' crates/perry-stdlib/src/readline.rs

echo "== deterministic call-order probe from source text =="
python3 - <<'PY'
from pathlib import Path
p = Path('crates/perry-stdlib/src/readline.rs')
s = p.read_text()
for name in ['js_readline_create_interface', 'js_readline_question', 'js_readline_on']:
    idx = s.find('pub extern "C" fn ' + name)
    print(f'{name} first occurrence: line={s[:idx].count(chr(10))+1 if idx!=-1 else None}')
print('ensure_gc_scanner_registered call sites:')
for m in [(s[i+1:i+s[i:].find('\n')+1],i) for i in range(len(s)) if s[i:].startswith('ensure_gc_scanner_registered()')]:
    print(f'line={s[:m[1]].count(chr(10))+1}')
PY

Repository: PerryTS/perry

Length of output: 19067


Register the readline GC scanner at interface construction.

ensure_gc_scanner_registered() is only called from the public .question() and .on() paths, but ReadlineInterfaceState::new() stores NaN-boxed input/output values during create_interface_from_options()/js_readline_create_interface(). If a readline interface is created without calling .question() or .on() before a moving GC pass, the scanner is still unregistered and those pointers are unscanned. Move the call_once registration into the interface construction path.

🤖 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-stdlib/src/readline.rs` around lines 197 - 206, Move the
one-time GC scanner registration from the `.question()`/`.on()` usage paths into
`ReadlineInterfaceState::new()`, which is used by
`create_interface_from_options()` and `js_readline_create_interface()`. Ensure
`ensure_gc_scanner_registered()` runs during interface construction before
storing or exposing the NaN-boxed input/output values, while preserving the
existing `Once`-guarded registration behavior.

@proggeramlug
proggeramlug merged commit ddb0f07 into PerryTS:main Jul 27, 2026
29 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant