land #8880: restore the end/close arm on the stdin provider path - #8881
Merged
Conversation
`echo hi | claude -p "…"` never completes on main. #8861 fixed this by teaching three layers about `end` listeners; the batch landing kept the GC rooting of STDIN_END_CALLBACKS and STDIN_PULL_MODE, but dropped the `"end" | "close"` arm in stdin_on_op — the one that made it work. stdin_on_op is the provider the stdin object's native on/once/addListener delegate to: every registration that is NOT codegen's literal process.stdin.x(…) shape, i.e. 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 those registrations hit `_ => return` and were discarded, and the end half of its 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. The two tests that guarded this were dropped with the arm, which is why CI stayed green. Restored, asserting the provider entry points directly rather than through js_readline_stdin_on — the extern path kept working, so it cannot catch this. Sabotage-checked. Bundle: 4/4 hang before, ~7s after. readline suite 21/21.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe stdin provider now handles ChangesStdin provider listener lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ 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 |
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
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.
Lands #8880 — restores the
end/closearm on the stdin listener provider path.The fix is correct and needed.
stdin_on_op/stdin_off_opare what the stdin object's nativeon/once/addListenerdelegate to — every registration that is not codegen's literalprocess.stdin.x(…)shape, i.e. 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, so those registrations hit_ => returnand were silently discarded, and theendhalf ofrace(once("end"), timeout(3000))could never win.Verified on
mainbefore merging:stdin_off_ophas onlydata/readablearms — noend/close. So the gap is real.One correction to the PR description
It attributes the gap to work being "lost when #8861 was batch-landed". That is not what happened, and the record is worth keeping straight:
0bf52fbbf) is byte-identical to fix(stdin): deliver piped stdin to-p— loweronce, keep everyendlistener, buffer pull-mode bytes #8861's head today — it did not move after I fetched it.stdin_on_opin fix(stdin): deliver piped stdin to-p— loweronce, keep everyendlistener, buffer pull-mode bytes #8861 as submitted has exactly"data"/"readable"/"keypress"/_ => return. The"end" | "close"arm was never present.provider_path_registers_end_listenersnorprovider_path_removes_end_listenersappears anywhere in fix(stdin): deliver piped stdin to-p— loweronce, keep everyendlistener, buffer pull-mode bytes #8861 (0 occurrences).mod_tests.rssplit I did while landing preserved every test: 15#[test]in fix(stdin): deliver piped stdin to-p— loweronce, keep everyendlistener, buffer pull-mode bytes #8861'smod.rs, 15 acrossmod.rs+mod_tests.rsonmain.So this is completing #8861's third layer, not restoring something dropped. #8861 landed the two layers it actually contained — the
STDIN_END_CALLBACKSGC rooting (which I added while landing, since the new list was missing from the root scanner) andSTDIN_PULL_MODE.Where I was incomplete: while landing #8864 I checked that the
"end" | "close"arm survived atreadline/mod.rs:1651and reported the interaction as safe. That line is the syntactic extern's arm, not the provider's. Checking one of two paths and reporting on both is the gap worth recording.Validation
provider_path_registers_end_listeners,provider_path_removes_end_listeners— and they assert the provider entry points directly, so this cannot silently regress againperry-stdlib124,perry-runtime2716 — 0 failedSummary by CodeRabbit
Bug Fixes
endandcloseevents when stdin is accessed through aliases or passed as a parameter.Tests