Skip to content

Fix Windows in-process Node E2E cleanup hang#1983

Closed
roji wants to merge 3 commits into
mainfrom
roji-fix-windows-node-inprocess-e2e-cleanup
Closed

Fix Windows in-process Node E2E cleanup hang#1983
roji wants to merge 3 commits into
mainfrom
roji-fix-windows-node-inprocess-e2e-cleanup

Conversation

@roji

@roji roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • track the Node E2E replay proxy child process in CapiProxy and make shutdown deterministic (bounded /stop request + process-exit wait + forced kill fallback)
  • add a Windows in-process-only fallback in Node E2E harness cleanup to force-stop the SDK client if graceful stop hangs
  • keep behavior unchanged for other transports/OSes while preventing afterAll teardown deadlocks in session.e2e.test.ts

Validation

  • attempted targeted run: cd nodejs && COPILOT_SDK_DEFAULT_CONNECTION=inprocess npm test -- test/e2e/session.e2e.test.ts
  • blocked locally because dependencies cannot be restored in this environment (@github/copilot@1.0.71-0 not accessible from packagefeedproxy)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 85a6b886-b234-446d-8719-8c7beb7ee03d
Copilot AI review requested due to automatic review settings July 14, 2026 15:03
@roji
roji requested a review from a team as a code owner July 14, 2026 15:03
@SteveSandersonMS

SteveSandersonMS commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@roji Since this PR doesn't change any code that's specific to inproc mode, do you have a sense of whether there is a separate inproc cleanup issue? No objections to this PR doing more cleanup if it's helpful but want to check whether there was an inproc-specific issue that will likely remain.

Or maybe this does interact with something specific to inproc but in a subtle way?

@github-actions

This comment has been minimized.

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

Makes Node E2E teardown bounded and deterministic, particularly for Windows in-process transport.

Changes:

  • Adds timed client shutdown with force-stop fallback.
  • Tracks and terminates the replay proxy child process.
Show a summary per file
File Description
nodejs/test/e2e/harness/sdkTestContext.ts Adds Windows in-process cleanup fallback.
nodejs/test/e2e/harness/CapiProxy.ts Adds bounded proxy shutdown and process termination.

Review details

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

Comment thread nodejs/test/e2e/harness/sdkTestContext.ts Outdated
Comment thread nodejs/test/e2e/harness/CapiProxy.ts
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 85a6b886-b234-446d-8719-8c7beb7ee03d
@github-actions

This comment has been minimized.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 85a6b886-b234-446d-8719-8c7beb7ee03d
@roji

roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

@SteveSandersonMS There is an in-process-specific teardown angle here on Windows: in-process shutdown/disposal runs in the shared host process, so a stalled cleanup path is harder to recover from than stdio/tcp where we can just reap a child runtime process.\n\nThis PR also fixes a Windows process-tree issue in the proxy harness ( child on Windows can leave descendants alive unless we use ). Together these were the cleanup paths that matched the Windows in-process hang signature in CI.

@roji

roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

@SteveSandersonMS There is an in-process-specific teardown angle here on Windows: in-process shutdown/disposal runs in the shared host process, so a stalled cleanup path is harder to recover from than stdio/tcp where we can just reap a child runtime process.\n\nThis PR also fixes a Windows process-tree issue in the proxy harness (when launched through a shell on Windows, terminating only the parent can leave descendants alive unless we terminate the full tree). Together these matched the Windows in-process cleanup-hang signature in CI.

@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review

This PR only modifies Node.js E2E test harness files (nodejs/test/e2e/harness/CapiProxy.ts and sdkTestContext.ts) — no public SDK API surface is changed.

The changes are internal test infrastructure improvements (deterministic proxy shutdown, Windows force-stop fallback), which are specific to the Node.js test setup. Equivalent concerns may exist in other language test harnesses but those are independent maintenance items, not API consistency gaps introduced by this PR.

No cross-SDK consistency issues found.

Generated by SDK Consistency Review Agent for issue #1983 · sonnet46 343K ·

@roji

roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

@SteveSandersonMS Adding a bit more detail on the inproc-specificity.

In stdio/tcp mode, the runtime is out-of-proc (separate child process), so teardown has a hard escape hatch: dispose transport and reap/kill the child if graceful shutdown stalls.

In inprocess mode, runtime shutdown/disposal happens in the shared host process (the test worker itself), so if that path stalls there is no separate runtime process boundary to reap. That is why the force-stop fallback is scoped to Windows + inprocess, where this cleanup hang was observed.

Separately, the proxy process-tree fix is Windows-general: when launched through a shell, killing only the parent can leave descendants alive unless we terminate the full tree.

@roji

roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Since this PR doesn't change any code that's specific to inproc mode, do you have a sense of whether there is a separate inproc cleanup issue? No objections to this PR doing more cleanup if it's helpful but want to check whether there was an inproc-specific issue that will likely remain.

Yeah, IIUC the problem is that if there's a teardown stall in out-of-proc, it's not a big issue since we just kill the child process and move on. But with inproc we don't have that, so a stall causes a real hang in the main (only) process. So this is to make sure that we don't hang in inproc etc.

I'll take a look at the root cause though.

@roji

roji commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

@SteveSandersonMS see #1984 for the root cause fix. This PR may still make sense to ensure we don't get long test hangs in the future, but I'm happy to close it (I'm not 100%, but doing this may actually partially mask such hangs in the future).

@roji roji closed this Jul 14, 2026
@SteveSandersonMS

Copy link
Copy Markdown
Contributor

Cool, happy either way whether you want to merge this change or #1984.

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