Skip to content

Handle CLI server stdin EPIPE on teardown#4464

Open
cklin wants to merge 1 commit into
mainfrom
cklin/fix-cli-server-stdin-epipe
Open

Handle CLI server stdin EPIPE on teardown#4464
cklin wants to merge 1 commit into
mainfrom
cklin/fix-cli-server-stdin-epipe

Conversation

@cklin

@cklin cklin commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a flaky write EPIPE failure that intermittently fails the CLI integration tests on Windows (e.g. queries.test.ts → "should restart the database and run a query"). Related: #4456.

Root cause

codeQL.restartQueryServer also restarts the persistent CLI server process (codeql execute cli-server) via cliServer.restartCliServer(). During teardown, killProcessIfRunning() writes a ["shutdown"] request to the child's stdin and then synchronously calls stdin.end() + kill(). On Windows the buffered write can flush asynchronously and emit an 'error' (EPIPE) after the read end has already closed.

With no 'error' listener on that stream, Node treats it as an unhandled stream error, which jest then attributes to whichever test happened to be running — surfacing as a bare write EPIPE.

This is Windows-specific because the pipe-close/kill timing loses the in-flight write; on Linux the write drains first.

Fix

Attach an 'error' listener to the CLI server's stdin inside killProcessIfRunning(), scoped to teardown, before issuing the shutdown write. The process is killed and dereferenced (this.process = undefined) immediately afterwards, so the listener is short-lived and does not accumulate across restarts.

Scoping the listener to teardown (rather than attaching it once-per-process in launchProcess()) is deliberate:

  • Cannot mask real failures / hang the queue — command completion is tracked via the stdout null-terminator and process close/error events, never via stdin. Because the listener only exists during teardown, stdin errors during an active command retain their existing behavior and cannot be swallowed.
  • No listener leakkillProcessIfRunning() is fully synchronous, so it can't re-enter on the same process; each restart spawns a fresh child, and the old stream plus its lone listener are GC'd.
  • The existing synchronous try/catch (which only guards the synchronous throw from write()) and the new async 'error' listener are complementary and non-overlapping.

Diagnosis

Root cause was confirmed via two instrumented CI runs on a throwaway branch: instrumenting the query server showed it healthy, while instrumenting the CLI server captured the stdin 'error' immediately after killProcessIfRunning, and handling it turned the Windows lane green.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Handles intermittent Windows EPIPE failures when restarting the persistent CodeQL CLI server.

Changes:

  • Adds a per-process stdin error listener.
  • Logs and suppresses asynchronous stdin errors during teardown.
Show a summary per file
File Description
extensions/ql-vscode/src/codeql-cli/cli.ts Handles CLI server stdin errors.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread extensions/ql-vscode/src/codeql-cli/cli.ts Outdated
The CLI server process's stdin can emit an asynchronous 'error' during
teardown: killProcessIfRunning() writes a "shutdown" request and then
immediately end()s and kill()s the process, so on Windows the buffered
write can complete with EPIPE once the read end closes. With no listener
this became an unhandled error that faulted whichever operation was
running, causing the flaky "should restart the database and run a query"
failures in the Windows cli-integration tests (the restart command also
restarts the CLI server via cliServer.restartCliServer()).

Attach an 'error' listener to the CLI server's stdin inside
killProcessIfRunning(), scoped to teardown, before issuing the shutdown
write. The process is killed and dereferenced immediately afterwards, so
the listener is short-lived and does not accumulate. Because it only
exists during teardown, stdin errors during an active command retain
their existing behaviour and cannot be masked.

Co-authored-by: Copilot <[email protected]>
Copilot-Session: 90105d62-02ab-405e-9c95-10466c971814
@cklin
cklin force-pushed the cklin/fix-cli-server-stdin-epipe branch from be2e207 to a2c5cc8 Compare July 16, 2026 22:31
@cklin
cklin requested a review from Copilot July 16, 2026 22:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@cklin
cklin marked this pull request as ready for review July 16, 2026 22:57
@cklin
cklin requested a review from a team as a code owner July 16, 2026 22:57
@cklin
cklin requested a review from josefs July 16, 2026 22:58
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.

3 participants