Skip to content

Clear the CLI startup timeout when we are told to stop#1046

Open
igfoo wants to merge 1 commit intogithub:mainfrom
igfoo:igfoo/timeout
Open

Clear the CLI startup timeout when we are told to stop#1046
igfoo wants to merge 1 commit intogithub:mainfrom
igfoo:igfoo/timeout

Conversation

@igfoo
Copy link
Copy Markdown
Member

@igfoo igfoo commented Apr 8, 2026

With index.ts as

import { CopilotClient } from "@github/copilot-sdk";

const client = new CopilotClient();
await client.start();
await client.stop();

running npx tsx index.ts takes more than 10 seconds. This is problematic for scripts that run quick queries, or even decide to run no queries after starting a client.

This is due to the 10 second timeout that is used to declare the CLI server failed to start.

With this PR, the above takes less than a second.

I can't run the npm test tests locally on main; I get as far as

 Test Files 3 passed (30)
      Tests 72 passed (72)
   Start at 17:36:49
   Duration 624.16s

and then it seemingly makes no further progress. But I get the same on my branch, and I assume that CI has run them.

Otherwise we won't terminate until the 10 seconds are up.
@igfoo igfoo marked this pull request as ready for review April 8, 2026 17:03
@igfoo igfoo requested a review from a team as a code owner April 8, 2026 17:03
Copilot AI review requested due to automatic review settings April 8, 2026 17:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a Node.js SDK shutdown delay by tracking the “CLI startup timeout” timer and clearing it when the client is stopped, so short-lived scripts don’t remain alive waiting for the 10s startup timeout to elapse.

Changes:

  • Track the CLI startup timeout handle on CopilotClient (cliStartTimeout).
  • Clear the pending startup timeout during stop() and forceStop().
  • Store the timeout handle when starting the CLI server.
Show a summary per file
File Description
nodejs/src/client.ts Track and clear the CLI startup timeout so stop/forceStop can exit promptly instead of being held by a pending 10s timer.

Copilot's findings

Comments suppressed due to low confidence (2)

nodejs/src/client.ts:1538

  • cliStartTimeout is set here but never cleared when the CLI successfully starts or when startup fails (e.g., error/exit handlers). If start() throws and the caller does not invoke stop(), this pending timeout can keep the Node.js event loop alive for up to 10 seconds. Consider clearing+nulling this.cliStartTimeout on all completion paths (resolve/reject), and also clearing any existing timeout before assigning a new one to avoid orphaned timers on concurrent start() calls.
            // Timeout after 10 seconds
            this.cliStartTimeout = setTimeout(() => {
                if (!resolved) {
                    resolved = true;
                    reject(new Error("Timeout waiting for CLI server to start"));

nodejs/src/client.ts:624

  • The clearTimeout + nulling logic is duplicated in both stop() and forceStop(). To reduce the chance of the two paths diverging over time, consider extracting this into a small private helper (e.g., clearCliStartTimeout()) and calling it from both places.
        if (this.cliStartTimeout) {
            clearTimeout(this.cliStartTimeout);
            this.cliStartTimeout = null;
        }
  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines +544 to +547
if (this.cliStartTimeout) {
clearTimeout(this.cliStartTimeout);
this.cliStartTimeout = null;
}
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

There’s now important cleanup logic for cliStartTimeout, but there are currently no unit tests covering stop()/forceStop() behavior (this file’s tests mainly use forceStop() as cleanup). Adding a small unit test with fake timers to assert that stop() (and forceStop()) clears a pending startup timeout would help prevent regressions where short-lived scripts hang for ~10s again.

This issue also appears in the following locations of the same file:

  • line 621
  • line 1534

Copilot uses AI. Check for mistakes.
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.

2 participants