diff --git a/changelog.d/8879-stdin-end-provider-arm.md b/changelog.d/8879-stdin-end-provider-arm.md new file mode 100644 index 0000000000..18e5245476 --- /dev/null +++ b/changelog.d/8879-stdin-end-provider-arm.md @@ -0,0 +1,38 @@ +Restored the `end`/`close` arm on the stdin listener **provider** path, which +was lost when #8861 was batch-landed. Without it `echo hi | claude -p "…"` +never completes. + +#8861 fixed piped stdin for `-p` by teaching three layers about `end` +listeners. Two of them landed: the GC rooting of `STDIN_END_CALLBACKS`, and +`STDIN_PULL_MODE`. The third — the `"end" | "close"` arm in `stdin_on_op` — +did not, and it is the one that made the fix work. + +`stdin_on_op` is the provider that the stdin object's native +`on` / `once` / `addListener` methods delegate to: every registration that is +**not** codegen's literal `process.stdin.x(…)` shape. That means an alias +(`const s = process.stdin; s.once("end", …)`) or stdin passed as a parameter +(`helper(process.stdin)`). Claude Code's print-mode reader is exactly the +parameter form — `X71(process.stdin, 3000)`, then `stream.once("end", …)` on +the parameter — so those registrations fell into `_ => return` and were +silently discarded. The `end` half of the reader's +`race(once("end"), timeout(3000))` could never win. + +`stdin_off_op` gets the mirror arm. #8864 removed the `end`/`close` clause from +the removal path, so a provider-registered listener could be added but never +removed — `removeListener` / `off` leaked it and the pump kept a stale callback +pointer. + +The two tests that guarded this — `provider_path_registers_end_listeners` and +`provider_path_removes_end_listeners` — were dropped alongside the arm, which is +why CI stayed green through the regression. Both are restored here. They assert +the provider entry points (`stdin_on_op` / `stdin_off_op`) **directly** rather +than going through the `js_readline_stdin_on` extern, because the extern path +kept working the whole time and therefore cannot catch this. Sabotage-checked: +deleting the arm again fails `provider_path_registers_end_listeners`. + +Measured on the claude-code 2.1.112 bundle, `echo hello | cc -p "…"`: + +| build | result | +|---|---| +| before this fix | 4/4 hang (120 s timeout, no output) | +| with the arm restored | completes in ~7 s | diff --git a/crates/perry-stdlib/src/readline/mod.rs b/crates/perry-stdlib/src/readline/mod.rs index a0a0069c1b..d94fd177e2 100644 --- a/crates/perry-stdlib/src/readline/mod.rs +++ b/crates/perry-stdlib/src/readline/mod.rs @@ -376,6 +376,27 @@ extern "C" fn stdin_on_op(name_ptr: *const u8, name_len: usize, cb: i64, _once: v.push(cb); } } + // #8861: `end`/`close` MUST be handled here, not only in the syntactic + // `js_readline_stdin_on` extern. + // + // This provider is what the stdin OBJECT's native `on`/`once`/ + // `addListener` methods delegate to — i.e. every registration that does + // not match codegen's literal `process.stdin.x(…)` pattern: an alias + // (`const s = process.stdin; s.once("end", …)`) or stdin passed as a + // parameter (`helper(process.stdin)`). That is exactly what Claude + // Code's print-mode reader does: `X71(process.stdin, 3000)`, then + // `stream.once("end", …)` on the parameter inside. + // + // Falling into the `_ => return` below silently discards those + // listeners. Node fires the direct, aliased and parameter forms alike; + // without this arm perry fires only the direct one, so the `end` half + // of the reader's `race(once("end"), timeout(3000))` can never win and + // `echo hi | claude -p …` never completes. + "end" | "close" => { + if let Ok(mut v) = STDIN_END_CALLBACKS.lock() { + v.push(cb); + } + } _ => return, } try_register_pump(); @@ -410,6 +431,21 @@ extern "C" fn stdin_off_op(name_ptr: *const u8, name_len: usize, cb: i64) { v.retain(|r| *r != cb); } } + // Mirror of the `end`/`close` arm in `stdin_on_op`. Without it a + // listener registered through the provider path can be added but never + // removed, so `removeListener`/`off` leaks it and the pump keeps a + // stale callback pointer. + "end" | "close" => { + if let Ok(mut v) = STDIN_END_CALLBACKS.lock() { + v.retain(|r| *r != cb); + } + CLOSE_CALLBACK.with(|slot| { + let mut slot = slot.borrow_mut(); + if *slot == Some(cb) { + *slot = None; + } + }); + } _ => {} } } diff --git a/crates/perry-stdlib/src/readline/mod_tests.rs b/crates/perry-stdlib/src/readline/mod_tests.rs index f93865beb4..5d3e0d5b78 100644 --- a/crates/perry-stdlib/src/readline/mod_tests.rs +++ b/crates/perry-stdlib/src/readline/mod_tests.rs @@ -77,6 +77,55 @@ fn every_stdin_end_listener_fires() { ); } +/// The PROVIDER path — `stdin_on_op` / `stdin_off_op` — must handle +/// `end`/`close` too, not just the syntactic `js_readline_stdin_on` extern +/// that `every_stdin_end_listener_fires` above covers. +/// +/// The provider is what the stdin object's native `on`/`once`/`addListener` +/// delegate to, i.e. every registration that is NOT codegen's literal +/// `process.stdin.x(…)` shape: an alias, or stdin passed as a parameter. +/// Claude Code's print-mode reader is the parameter form — `X71(process.stdin, +/// 3000)` then `stream.once("end", …)` inside — so without these arms its +/// `race(once("end"), timeout(3000))` can never resolve via `end` and +/// `echo hi | claude -p …` never completes. +/// +/// These two tests guard arms that have now been lost twice, which is why they +/// assert the provider entry points directly rather than going through the +/// extern. +#[test] +fn provider_path_registers_end_listeners() { + let _g = reset(); + let a = data_counter_callback(); + let b = data_counter_callback(); + stdin_on_op(b"end".as_ptr(), 3, a, 0); + stdin_on_op(b"close".as_ptr(), 5, b, 1); + assert_eq!( + STDIN_END_CALLBACKS.lock().map(|v| v.len()).unwrap_or(0), + 2, + "aliased end/close registrations must reach the end-listener list" + ); + EOF_REACHED.store(true, Ordering::Release); + js_readline_process_pending(); + assert_eq!( + DATA_COUNT.with(|n| *n.borrow()), + 2, + "listeners registered through the provider must fire at EOF" + ); +} + +#[test] +fn provider_path_removes_end_listeners() { + let _g = reset(); + let cb = data_counter_callback(); + stdin_on_op(b"end".as_ptr(), 3, cb, 0); + stdin_off_op(b"end".as_ptr(), 3, cb); + assert_eq!( + STDIN_END_CALLBACKS.lock().map(|v| v.len()).unwrap_or(0), + 0, + "removing an aliased end listener must clear it" + ); +} + /// An `on("readable")` listener puts stdin in paused/pull mode, where the /// fd-0 reader must buffer bytes for `process.stdin.read()` instead of /// routing them to readline's line queue (which `read()` never drains).