fix(sea): run forked helper scripts directly instead of spawning a new session#26366
fix(sea): run forked helper scripts directly instead of spawning a new session#26366noxymon wants to merge 6 commits into
Conversation
Batches printable characters during raw paste events to avoid excessive generator yields that freeze the Ink renderer. Fixes the bracketed paste hang issue described in H1.
…w session
child_process.fork() uses process.execPath as the Node.js interpreter, which
in a SEA build is the gemini binary itself. So fork('helper.js', [pid]) calls
made by helper modules — notably node-pty's _getConsoleProcessList() during
Windows ConPTY cleanup — would launch a full second gemini session instead
of running the requested script. The second session inherits GEMINI_DEBUG_LOG_FILE
and other env vars, runs without the original CLI flags, and on Windows can
even surface a duplicate UI session with approvalMode reset to the default.
Detect this case at SEA entry: when process.send is set (an IPC channel
exists, indicating fork()) and argv contains a .js script path other than
the binary itself, normalize argv to the shape the helper expects
([binary, script, ...args]) and load the script with createRequire so it
can resolve disk-based dependencies that the SEA's built-in require() does
not support.
Spawns a tiny helper script via child_process.fork() with execPath set to the SEA binary under test, mirroring how node-pty's windowsPtyAgent._getConsoleProcessList() invokes conpty_console_list_agent.js during ConPTY teardown. The helper sends an IPC message back; the test passes if and only if the message is received within the timeout. Without the fix the SEA spawns a full second gemini session and the IPC message never arrives (test times out, exits 1). With the fix the helper runs and replies in tens of milliseconds (test exits 0). Usage: node sea/sea-launch.fork.integration.test.cjs <path-to-binary>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the Single Executable Application (SEA) build where child_process.fork() calls inadvertently spawn secondary gemini sessions instead of executing the intended helper scripts. By detecting the IPC channel at startup and normalizing arguments, the application now correctly delegates to the helper script. Additionally, the PR includes a performance optimization for the CLI UI to handle large paste operations more efficiently. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces performance optimizations for large text pastes by batching printable characters and adds logic to correctly handle child_process.fork() calls within Single Executable Application (SEA) builds. Review feedback points out a critical flaw in the keypress batching implementation that could break ANSI escape sequences, such as arrow keys, and suggests using absolute paths in the fork detection logic to ensure consistent behavior across different working directories.
…eck and createRequire Two robustness improvements raised in code review: 1. process.execPath is always absolute, but argv[i] from a forked invocation can be relative (e.g. 'test-binaries/gemini-debug.exe'). String-comparing the two would fail to skip the binary's own path duplicate. Resolve the candidate to an absolute path with path.resolve before the comparison. 2. Module.createRequire requires an absolute path or file URL — relative paths resolve against process.cwd(), which can change mid-startup (e.g. during worktree setup). Use the resolved absolute scriptPath for both createRequire and the recursive scriptRequire(scriptPath) call.
…-batching dispatcher The data-listener optimization in createDataListener batches consecutive printable characters into a single 'paste' event so large clipboard pastes do not trigger thousands of React re-renders. Two correctness issues: 1. Escape-sequence interleaving: in '\x1b[A' (Up arrow) the bytes '[' and 'A' are printable (>= 0x20) and were being collected into a 'paste' batch while the parser was mid-CSI, leaving the parser stuck waiting for a sequence end and silently swallowing arrow keys, function keys, and other ESC-prefixed inputs. Track an inEscape flag plus the intro byte so we feed sequence bytes to the parser instead of batching them, and exit escape mode on either a parser-emitted event or a sequence-typical final byte (CSI/SS3 final 0x40-0x7E, OSC BEL/ST). The state persists across data chunks since a sequence may straddle them. 2. Single-character / fast-typing inputs were being misclassified as paste: any printable run of length > 1 (e.g. typing 'abc' or a 3-char CJK input) produced a 'paste' event instead of individual keypress events, breaking the keypress contract relied on by tests and consumers. Introduce a PASTE_BATCH_THRESHOLD of 32 characters; below it, feed each char through the parser so individual keypress events are emitted; at or above it, emit the single 'paste' event that solves the actual UI hang case.
|
Thanks for the review. Both points addressed in Comment 2 — Comment 1 —
While fixing this I also found a related correctness bug not in the original review: small printable runs like the test inputs Verified end-to-end:
|
gemini-cli
Bot
added
priority/p1
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
1 participant
Summary
In the SEA (Single Executable Application) build,
child_process.fork(modulePath, args)usesprocess.execPathas the Node.js interpreter — which isgemini.exeitself. Anyfork()call from app code or a transitive dependency therefore launches a secondgeminisession in a child process instead of executing the requested helper script.The most visible victim is
@lydell/node-pty'sWindowsPtyAgent._getConsoleProcessList(), whichfork()sconpty_console_list_agent.jsduring ConPTY teardown. On Windows, this means everyrun_shell_commandinvocation in the SEA build can spawn a secondgeminisession, with debug-log interleaving, lost CLI flags (e.g.--yolobecomesautoEdit), and anode-pty5-second timeout instead of prompt console-handle reaping.This PR detects fork-style invocation at SEA entry and runs the requested script directly.
Details
In
sea/sea-launch.cjs, before any other startup work inmain():typeof process.send === 'function'— the runtime indicator that this process was spawned with an IPC channel (i.e. viafork()). Note:NODE_CHANNEL_FDis no longer set on the child env in modern Node.js, soprocess.sendis the reliable signal.process.argv[1..]for the first.jsscript that is not the binary itself. The SEA inserts a duplicate ofexecPathatargv[1](the script-path slot a non-SEA Node.js would have), so forfork(helper.js, [pid])the SEA child seesargv = [binary, binary, helper.js, pid].process.argvto[binary, script, ...remainingArgs]so the helper sees argv in the same positions a regular Node fork would deliver — important for helpers that readprocess.argv[2](whichconpty_console_list_agent.jsdoes for the shell PID).Module.createRequire(scriptPath). The SEA's built-inrequire()only resolves built-in modules (Node SEA docs), socreateRequirerooted at the script path is required for the helper to load its native-addon dependencies.returnrather than falling through togeministartup — the parent's IPC timeout will recover, and we never want to spawn a second session.The fix is generic: it benefits any
fork()-using helper, not justnode-pty.Related Issues
Closes #26365
How to Validate
A standalone integration test is included as
sea/sea-launch.fork.integration.test.cjs. It mirrors whatnode-ptydoes —fork()s a tiny helper script using the SEA binary asexecPathand waits for the helper's IPC reply.Expected after this PR (PASS):
Without this PR (FAIL — bug present):
To verify the BEFORE behavior locally, build a binary from
main, run the integration test against it, and confirm it fails the same way.End-to-end on Windows:
gemini.exe --yolo.run shell command: pwd).gemini.exePID exists;GEMINI_DEBUG_LOG_FILEshows a single session;approvalModestaysyolo.Pre-Merge Checklist
sea/sea-launch.fork.integration.test.cjs(integration) and updatedsea/sea-launch.test.jsprocess.sendis set, which is never true for normal user invocations