Fix stale-session in-process Node E2E teardown stall#1984
Closed
roji wants to merge 4 commits into
Closed
Conversation
Bound in-process per-session abort/disconnect teardown in CopilotClient.stop and skip already-disconnected sessions to avoid unbounded teardown awaits. Adds shutdown coverage for connected-session filtering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc
Contributor
There was a problem hiding this comment.
Pull request overview
Bounds Node.js in-process session teardown to prevent CopilotClient.stop() stalls.
Changes:
- Adds abort and disconnect timeouts.
- Skips teardown calls for disconnected sessions.
- Adds connected-session filtering coverage.
Show a summary per file
| File | Description |
|---|---|
nodejs/src/client.ts |
Adds bounded session teardown. |
nodejs/src/session.ts |
Exposes internal disconnected state. |
nodejs/test/client.test.ts |
Tests connected-session filtering. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
This comment has been minimized.
This comment has been minimized.
Keep existing non-in-process disconnect behavior while adding fake-timer coverage for stalled in-process abort and disconnect cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc
This comment has been minimized.
This comment has been minimized.
Report abort timeout failures from client teardown and make the shared E2E harness fail after completing proxy and directory cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc
roji
marked this pull request as draft
July 14, 2026 16:18
This comment has been minimized.
This comment has been minimized.
Revert product cleanup behavior changes and remove stale disconnected sessions only from the Node E2E harness before teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 05955426-95a4-4f54-b05f-1b14cc3099bc
roji
changed the title
Contributor
Cross-SDK Consistency Review ✅This PR modifies only Findings:
No cross-SDK consistency issues found. 🎉
|
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
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
CopilotClient.stop()and session lifecycle code are unchangedCopilotSession.disconnect()leaves disconnected objects in the client registry. In-processstop()then aborts every registry entry; aborting an already-destroyed session can leave the outerPromise.allSettledwaiting indefinitely. This test-harness-only change prevents that stale-session fan-out in the affected E2E teardown path.This complements #1983's containment by removing the known test teardown trigger. It does not mask cleanup failures returned by
stop().Validation
cd nodejs && COPILOT_SDK_DEFAULT_CONNECTION=inprocess npm test -- test/e2e/session.e2e.test.ts(36 passed, 2 skipped)