test: synchronize stdin lifecycle input with child readiness #9806
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Make the stdin lifecycle parity fixture wait for each child to finish its toggle | ||
| and GC churn before sending the second input chunk. Removing the four fixed | ||
| 2.5-second waits lets the Node oracle finish within the suite's 10-second budget. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,10 @@ function runRole(name: string, onFirst: (s: any) => void, doChurn: boolean): voi | |
| console.log(name + " phase1: true"); | ||
| onFirst(s); | ||
| if (doChurn) console.log(name + " churn: " + (churn(300000) > 0)); | ||
| // The parent sends TWO only after the toggle and all churn complete. | ||
| // A fixed delay cannot prove this ordering and four sequential 2.5s | ||
| // waits alone exceed the parity suite's 10s per-process budget (#9783). | ||
| console.log(name + " ready: true"); | ||
| } else if (phase === 1 && text.indexOf("TWO") >= 0) { | ||
| clearInterval(ticker); | ||
| finish(name + " phase2: true"); | ||
|
|
@@ -110,17 +114,33 @@ if (role === "unref-ref") { | |
| new Promise<void>((resolve) => { | ||
| const child = spawn(process.execPath, childArgs, { | ||
| env: { ...process.env, [ROLE_ENV]: name }, | ||
| stdio: ["pipe", "pipe", "inherit"], | ||
| }); | ||
|
Comment on lines
+117
to
118
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🤖 get_repo_knowledge executed:
Length of output: 12018 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n test-files/test_gap_9676_stdin_unref_ref_keeps_reader.ts
printf '%s\n' '--- version and process-related declarations ---'
rg -n --glob 'package.json' --glob '*.json' --glob '*.yml' --glob '*.yaml' --glob '*.md' 'node(js)?|engines|process\.exit|test_gap_9676|phase2|watchdog' .Repository: PerryTS/perry Length of output: 50370 🏁 Script executed: #!/bin/bash
set -eu
log=/tmp/coderabbit-shell-logs/shell-output-Tvqdsg
printf '%s\n' '--- target-file excerpt from prior output ---'
sed -n '1,180p' "$log"
printf '%s\n' '--- focused repository version/config matches ---'
rg -n --glob '.node-version' --glob 'package.json' --glob 'README.md' --glob '*.md' --glob '*.json' \
'26\.5\.0|26\.5\.1|process\.exit\(|function finish|test_gap_9676_stdin_unref_ref_keeps_reader' \
. | head -n 120Repository: PerryTS/perry Length of output: 35945 🌐 Web query:
💡 Result: In Node.js, including v26.5.0, the process.exit method terminates the process synchronously and immediately [1]. This behavior forces the process to exit as quickly as possible, even if there are asynchronous operations pending that have not completed, such as I/O operations to process.stdout or process.stderr [1]. Because writes to process.stdout are often asynchronous and can occur over multiple ticks of the Node.js event loop, calling process.exit frequently results in pending writes to these streams being abandoned, which can cause output to be truncated or lost [1]. The official documentation advises against using process.exit explicitly [1]. Instead, it recommends the following best practices: 1. Allow the process to exit naturally by ensuring there is no additional work pending in the event loop [1]. 2. Set the process.exitCode property to define the desired exit code, allowing the process to terminate gracefully once all tasks are complete [1]. 3. If an error condition requires termination, throwing an uncaught error is generally considered safer than calling process.exit [1]. If you absolutely must use process.exit and need to ensure output is flushed, some developers have used workarounds such as setting the underlying handles to blocking mode (e.g., process.stdout._handle.setBlocking(true)), though this is not standard or guaranteed to be reliable across all platforms and use cases [2]. Relying on the natural termination of the event loop remains the recommended approach [1]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target fixture ---'
cat -n test-files/test_gap_9676_stdin_unref_ref_keeps_reader.ts | sed -n '1,180p'
printf '%s\n' '--- pinned Node version ---'
for f in .node-version package.json test-parity/node_suite_baseline.json; do
if [ -f "$f" ]; then
echo "[$f]"
rg -n '26\.5|engines|node' "$f" | head -n 30
fi
doneRepository: PerryTS/perry Length of output: 10116 Flush the child’s final output before exiting. When the child receives Proposed fix function finish(line: string): void {
- console.log(line);
- process.exit(0);
+ process.stdout.write(line + "\n", () => process.exit(0));
}🤖 Prompt for AI AgentsSource: MCP tools |
||
| let settled = false; | ||
| let output = ""; | ||
| let sentSecond = false; | ||
| child.stdout!.on("data", (chunk: any) => { | ||
| process.stdout.write(chunk); | ||
| output += String(chunk); | ||
| // stdout may split the readiness line across arbitrary chunks. | ||
| if (!sentSecond && output.includes(name + " ready: true\n")) { | ||
| sentSecond = true; | ||
| try { | ||
| child.stdin!.write("TWO\n"); | ||
| } catch { | ||
| /* child already gone */ | ||
| } | ||
| } | ||
| }); | ||
| const watchdog = setTimeout(() => { | ||
| if (settled) return; | ||
| settled = true; | ||
| console.log(name + " exit: WATCHDOG"); | ||
| child.kill("SIGKILL"); | ||
| resolve(); | ||
|
Comment on lines
137
to
140
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🤖 get_repo_knowledge executed:
Length of output: 7690 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- changed hunk ---'
git diff -- test-files/test_gap_9676_stdin_unref_ref_keeps_reader.ts
printf '%s\n' '--- file outline ---'
ast-grep outline test-files/test_gap_9676_stdin_unref_ref_keeps_reader.ts
printf '%s\n' '--- relevant source ---'
cat -n test-files/test_gap_9676_stdin_unref_ref_keeps_reader.ts | sed -n '1,180p'
printf '%s\n' '--- runtime/version declarations ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' '(engines|node-version|NODE_VERSION|process\.versions\.node|\.nvmrc|volta)' package.json .nvmrc .node-version .github 2>/dev/null || trueRepository: PerryTS/perry Length of output: 14760 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- pinned Node version ---'
cat .node-version
printf '%s\n' '--- fixture references and output assertions ---'
rg -n -C 4 'test_gap_9676_stdin_unref_ref_keeps_reader|unref-ref-churn|exit: WATCHDOG|phase2: true|done' --glob '!node_modules' --glob '!dist' --glob '!build' .
printf '%s\n' '--- relevant workflow/test runner context ---'
rg -n -C 5 'test-files|test_gap_' .github/workflows/test.yml scripts package.json 2>/dev/null | head -240Repository: PerryTS/perry Length of output: 50370 🌐 Web query:
💡 Result: In Node.js, the 'close' event is emitted on a ChildProcess instance only after the process has terminated and all of its stdio streams have been closed [1][2][3]. Key distinctions and technical details: 1. Relationship to 'exit': The 'close' event is distinct from the 'exit' event [1][4]. The 'exit' event is emitted as soon as the process terminates, at which point stdio streams may still be open [2][3]. The 'close' event will always be emitted after the 'exit' event (if the process exited) or after an 'error' event (if the process failed to spawn) [1][5]. 2. Stdio and 'close': Because multiple processes can share the same stdio streams, the 'close' event serves as a guarantee that all streams associated with that specific child process have finished [1][2]. If you are working with piped output or need to ensure all data has been processed, listening for 'close' is more reliable than 'exit' [6]. 3. Flush Behavior and process.exit: A critical nuance exists regarding stdout flushing. When a process terminates naturally, Node.js allows time for stdio streams to drain. However, calling process.exit forces the process to terminate synchronously and immediately [7][8]. This can cause the process to exit before asynchronous write operations to stdout or stderr are completed, potentially resulting in data loss [7][8]. To ensure data is flushed, it is generally recommended to avoid explicit calls to process.exit and instead allow the Node.js event loop to empty naturally, or to explicitly handle stream completion before termination [7][9][8]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- pinned Node version ---'
tr -d '\r\n' < .node-version
printf '\n%s\n' '--- exact fixture references ---'
rg -n -C 6 --glob '!node_modules' --glob '!dist' --glob '!build' \
'test_gap_9676_stdin_unref_ref_keeps_reader|9676_stdin|unref-ref-churn|exit: WATCHDOG' . \
| head -240
printf '%s\n' '--- runner contracts for test-files ---'
rg -n -C 4 --glob '!node_modules' --glob '!dist' --glob '!build' \
'test-files/|test-files\b|run.*fixture|expected.*stdout|stdout.*expected' \
.github/workflows/test.yml scripts package.json 2>/dev/null \
| head -240Repository: PerryTS/perry Length of output: 24275 Wait for This branch resolves 🤖 Prompt for AI AgentsSource: MCP tools |
||
| }, WATCHDOG_MS); | ||
| // Drain the piped stdout before printing the role's exit summary. | ||
| child.on("close", (code) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(watchdog); | ||
|
|
@@ -134,14 +154,6 @@ if (role === "unref-ref") { | |
| /* child already gone */ | ||
| } | ||
| }, 120); | ||
| }); | ||
|
|
||
| (async () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the readiness marker out of fixture output.
Line 82 writes
name + " ready: trueto child stdout, and Line 123 forwards every stdout chunk. Each child therefore adds a readiness line to the parent output. This breaks the byte-identical output objective.Send readiness through a separate control channel, or remove only the complete marker with a line-buffered demultiplexer before forwarding output.
Also applies to: 123-123
🤖 Prompt for AI Agents