Skip to content

fix: wait for child process completion before parent exits#526

Merged
Brooooooklyn merged 7 commits intomainfrom
claude/fix-issue-493
Mar 27, 2026
Merged

fix: wait for child process completion before parent exits#526
Brooooooklyn merged 7 commits intomainfrom
claude/fix-issue-493

Conversation

@Claude
Copy link
Copy Markdown
Contributor

@Claude Claude AI commented Mar 26, 2026

@Claude Claude AI changed the title [WIP] Fix issue 493 in deepresearch project fix: wait for child process completion before parent exits Mar 26, 2026
@Claude Claude AI requested a review from Brooooooklyn March 26, 2026 10:09
@Brooooooklyn
Copy link
Copy Markdown
Member

@claude[agent]

process-completion › child process completes before parent exits
  
    __tests__/process-completion.spec.ts:31
  
     30:     // The fixture exits with code 0 after writing the file   
     31:     t.is(result.status, 0, "Process should exit with code 0");
     32:     t.falsy(result.error, "No spawn error should occur");     
  
    Process should exit with code 0
  
    Difference (- actual, + expected):
  
    - 1
    + 0
  
    › <anonymous> (__tests__/process-completion.spec.ts:31:7)
  
  
  
    process-completion › child process exit code is propagated to parent
  
    __tests__/process-completion.spec.ts:59
  
     58:                                                                         
     59:   t.is(result.status, 42, "Parent should exit with same code as child");
     60: });                                                                     
  
    Parent should exit with same code as child
  
    Difference (- actual, + expected):
  
    - 1
    + 42
  
    › <anonymous> (__tests__/process-completion.spec.ts:59:5)
  
    ─
  
    2 tests failed
  /Users/runner/work/oxc-node/oxc-node/packages/integrate-ava:
   ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  integrate-ava@ test: `ava`
  Exit status 1

@Claude
Copy link
Copy Markdown
Contributor Author

Claude AI commented Mar 26, 2026

@claude[agent]

process-completion › child process completes before parent exits

...

Fixed in 3600008. The test was invoking the TypeScript source file directly instead of using the built CLI binary from dist/index.js. Updated to match the pattern used in other tests.

@Claude Claude AI changed the title fix: wait for child process completion before parent exits Addressing PR comments Mar 26, 2026
@Brooooooklyn
Copy link
Copy Markdown
Member

@claude[agent] still failed:

 ✔ process-completion › child process completes before parent exits (11.4s)
  ✘ [fail]: process-completion › child process exit code is propagated to parent Parent should exit with same code as child
  ✔ stdin-tty › CLI properly handles stdin piping (6.1s)
  ✔ cli › CLI stack trace for stacktrace-esm.ts (16.6s)
  ✔ cli › CLI stack trace for stacktrace-esm.mts (10.5s)
  ✔ cli › CLI stack trace for stacktrace-cjs.cts (4s)
  ─

  process-completion › child process exit code is propagated to parent

  __tests__/process-completion.spec.ts:51

   50:                                                                         
   51:   t.is(result.status, 42, "Parent should exit with same code as child");
   52: });                                                                     

  Parent should exit with same code as child

  Difference (- actual, + expected):

  - 0
  + 42

  › <anonymous> (__tests__/process-completion.spec.ts:51:5)

  ─

  1 test failed
/home/runner/work/oxc-node/oxc-node/packages/integrate-ava:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  integrate-ava@ test: `ava`
Exit status 1
 ELIFECYCLE  Test failed. See above for more details.

@Claude
Copy link
Copy Markdown
Contributor Author

Claude AI commented Mar 26, 2026

@claude[agent] still failed:

 ✔ process-completion › child process completes before parent exits (11.4s)

...

Fixed in 5de025e. The Promise now resolves with the child's exit code instead of undefined. Clipanion's runExit uses the returned value to set the parent's exit code, so returning undefined was causing it to default to 0.

@Brooooooklyn Brooooooklyn marked this pull request as ready for review March 26, 2026 11:42
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5de025e105

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +18 to +24
const result = spawnSync(process.execPath, [CLI_PATH, FIXTURE_PATH, outputPath], {
encoding: "utf8",
env: {
...process.env,
NODE_OPTIONS: undefined,
},
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Run CLI with isolated stdio in completion regression test

spawnSync is invoked with default stdio (pipe), but the CLI under test spawns its child with stdio: "inherit"; that grandchild inherits the same pipe FDs, so this spawnSync call can block until the grandchild exits even if the CLI process exits too early. In that scenario the existsSync(outputPath) assertion still passes, so the test does not actually catch the regression it is meant to guard against.

Useful? React with 👍 / 👎.

@Brooooooklyn
Copy link
Copy Markdown
Member

@claude[agent] from reviewer comments:

spawnSync is invoked with default stdio (pipe), but the CLI under test spawns its child with stdio: "inherit"; that grandchild inherits the same pipe FDs, so this spawnSync call can block until the grandchild exits even if the CLI process exits too early. In that scenario the existsSync(outputPath) assertion still passes, so the test does not actually catch the regression it is meant to guard against.

@Claude
Copy link
Copy Markdown
Contributor Author

Claude AI commented Mar 26, 2026

@claude[agent] from reviewer comments:

spawnSync is invoked with default stdio (pipe), but the CLI under test spawns its child with stdio: "inherit"; that grandchild inherits the same pipe FDs, ...

Fixed in 21f6d75. Added stdio: 'ignore' to both tests to prevent spawnSync from blocking on the grandchild's stdio pipes. This ensures the tests only wait for the CLI process itself to exit, properly verifying that the CLI waits for its child to complete.

@Brooooooklyn Brooooooklyn changed the title Addressing PR comments fix: wait for child process completion before parent exits Mar 27, 2026
@Brooooooklyn Brooooooklyn merged commit 455c598 into main Mar 27, 2026
53 checks passed
@Brooooooklyn Brooooooklyn deleted the claude/fix-issue-493 branch March 27, 2026 02:38
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.

oxnode parent process can exit before child process finishes

2 participants