Handle version-details timeout and stalled connects during startup - #5523
Merged
Justin Grote (JustinGrote) merged 1 commit intoJun 18, 2026
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
There was a problem hiding this comment.
Pull request overview
This PR fixes two latent startup-path bugs in src/session.ts reported in #5521. Previously, if getVersionDetails() timed out, the cancellation token rejection would throw out of start() and bypass the intended "Unable to get version details!" failure handling. Additionally, connectFunc had no error or timeout handlers, meaning a stalled named-pipe or socket connection would hang indefinitely.
Changes:
- Wrap the version-details sendRequest in a try/catch/finally so that a timeout or transport failure returns undefined instead of throwing, and properly clean up the timer and CancellationTokenSource.
- Add an error handler and a 60-second connect timeout to connectFunc that destroys the socket and rejects with a clear, logged error message.
- Add unit tests for the getVersionDetails method covering both the rejection-to-undefined path and the normal success path, with shared test helpers extracted into makeManager(), setSendRequest(), and getVersionDetails().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/session.ts | Adds try/catch/finally to getVersionDetails() for graceful timeout handling with proper resource cleanup; adds error handler and 60s timeout to connectFunc for stalled connections. |
| test/core/session.test.ts | Adds describe("SessionManager.getVersionDetails") test suite with two tests and shared helper functions (makeManager, setSendRequest, getVersionDetails). |
Sorry, something went wrong.
Two latent startup-path bugs in `src/session.ts`, surfaced while investigating #5031 (neither caused it): - `getVersionDetails()` set a 60s timeout that only cancelled the token, so when it fired `sendRequest(...)` rejected and the `await` threw straight out of `start()` — the intended graceful failure handling was never reached. We now catch the rejection, log the underlying cause as a warning, and return `undefined` so the existing failure branch runs and surfaces a friendlier, more actionable message to the user. While here, fix the timer leak: we capture the `setTimeout` handle to `clearTimeout` it and `dispose()` the `CancellationTokenSource`. - `connectFunc` had no error or timeout handler, so a transport that never connected just hung with nothing actionable. We add an `error` handler and a 60s connect timeout that destroys the socket and rejects with a clear, logged message naming the pipe. Both paths clear the timer so a healthy but idle connection is never torn down. Adds unit tests for `getVersionDetails()` covering the reject-to- `undefined` path and the normal fast path. `connectFunc` is a private closure over a live `net` socket with no test seam, so I left it uncovered rather than add socket-mocking infra the repo deliberately avoids. Closes #5521. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Andy Jordan (andyleejordan)
force-pushed
the
andyleejordan/issue-5521-startup-version-details-timeout-throws-i-3a0780
branch
from
638fc37 to
eaa5931
Compare
June 16, 2026 15:39
Justin Grote (JustinGrote)
approved these changes
Jun 18, 2026
Justin Grote (JustinGrote)
left a comment
Copy link
Copy Markdown
Collaborator
There was a problem hiding this comment.
Tests pass on my machine. not sure if anyone ever hit this deadlock but a good find.
Sorry, something went wrong.
Justin Grote (JustinGrote)
deleted the
andyleejordan/issue-5521-startup-version-details-timeout-throws-i-3a0780
branch
June 18, 2026 03:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the two latent startup-path bugs in src/session.ts reported in #5521 (surfaced while investigating #5031; neither caused it).
1. getVersionDetails() timeout threw out of start()
The 60s timeout only cancelled the token, so when it fired sendRequest(...) rejected and the await threw straight out of start() — the intended "Unable to get version details!" handling was never reached. It now catches the rejection, logs it via ILogger, and returns undefined so the existing if (versionDetails === undefined) failure branch runs. Also fixes the original timer leak: the setTimeout handle is captured and clearTimeout'd, and the CancellationTokenSource is dispose()d in finally.
2. connectFunc had no error/timeout handler
A transport that never connected just hung with nothing actionable. Adds an error handler and a 60s connect timeout that destroy()s the socket and rejects with a clear, logged message naming the pipe. Both paths clearTimeout so a healthy but idle connection is never torn down. Works whether the transport is a named pipe or a socket.
Tests
Adds describe("SessionManager.getVersionDetails") in test/core/session.test.ts covering the reject-to-undefined path and the normal fast path. connectFunc is a private closure over a live net socket with no test seam, so it's left uncovered rather than adding socket-mocking infra the repo deliberately avoids.
Verification
Closes #5521.
🤖 Drafted by GitHub Copilot (Claude Opus 4.8) at Andy Jordan (@andyleejordan)'s request.