Bound relay drains so a wedged guest socket cannot hang wsl.exe - #41164
Open
yeelam-gordon wants to merge 2 commits into
Open
Bound relay drains so a wedged guest socket cannot hang wsl.exe#41164yeelam-gordon wants to merge 2 commits into
yeelam-gordon wants to merge 2 commits into
Conversation
…rvice ScopedRelay::Sync() joined the relay thread unconditionally, so a guest that never closed its stdout/stderr hvsocket left wsl.exe (install) and wslservice (import/export/resize) blocked forever. Add a bounded Sync(timeout) overload: wait for natural EOF up to the timeout (no truncation of slow-but-alive streams), otherwise signal the relay's exit event and join. The verdict is captured before draining, so bounding teardown is safe. Exec stdout/stderr workers become std::optional<ScopedRelay> to share the bounded path. Adds ScopedRelay unit tests proving the timeout returns bounded and that a stream completing before the deadline is not truncated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e76db014-5ee2-46f5-b078-42ad74b5197a
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses an indefinite-hang risk in wsl.exe teardown paths (install/import/export/resize) by introducing a bounded drain for relay threads, preventing ScopedRelay::Sync() from blocking forever when a guest stream never reaches EOF.
Changes:
- Added
ScopedRelay::Sync(timeout)and a default drain cap (c_relayDrainTimeout = 60s) to bound teardown while still draining fully when EOF arrives before the deadline. - Updated service/client teardown call sites to use the bounded drain for stdout/stderr relays.
- Added new Windows unit tests covering both the timeout (wedged stream) and full-drain (no truncation) behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/windows/common/relay.hpp |
Adds default drain timeout constant and new bounded ScopedRelay::Sync(timeout) API; makes ScopedRelay non-movable. |
src/windows/common/relay.cpp |
Implements bounded drain logic via a completion event + timeout-triggered cancellation before joining. |
src/windows/common/svccomm.cpp |
Applies bounded drain to client-side relays and switches exec stdout/stderr relays to std::optional<ScopedRelay> for unified handling. |
src/windows/service/exe/LxssUserSession.cpp |
Applies bounded drain to utility-VM error relays during export/import teardown. |
test/windows/ScopedRelayUnitTests.cpp |
Adds new unit tests validating timeout-bounded teardown and full-payload drain without truncation. |
test/windows/CMakeLists.txt |
Includes the new relay unit test file in the Windows test build. |
OneBlue
requested changes
Jul 24, 2026
OneBlue
left a comment
Collaborator
There was a problem hiding this comment.
Do we know why things get stuck in the guest in the first place ? If there's a socket that stays opened, that's what we should resolve
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e76db014-5ee2-46f5-b078-42ad74b5197a
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/windows/common/relay.cpp:463
- Sync(timeout) currently implements a total wait-for-completion: if the relay hasn’t reached EOF within
Timeout, it signalsm_exitEvent, which cancels any pending I/O and can truncate remaining stdout/stderr. This contradicts the PR description’s “slow-but-alive stream is still drained in full (no truncation)” guarantee for long drains (e.g. very large output that would eventually reach EOF after >60s). Consider either (a) changing the timeout semantics to an inactivity/progress timeout (reset when bytes are relayed), or (b) updating the PR description / call-site comments to explicitly acknowledge that output may be truncated after the cap.
if (!m_completed.wait(timeoutMs))
{
m_exitEvent.SetEvent();
}
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.
Why (the symptom)
wsl.exeinstall can hang forever and never exit (the same teardown path is also used by import / export / resize). Seen intermittently in automated test runs on heavily loaded machines. Tracked as 62543726 (duplicates 63009527, 63219697).Root cause
At teardown,
ScopedRelay::Sync()does an unconditionalm_thread.join(). That relay thread only exits when the Linux guest closes its stdout/stderr hvsocket. If the guest never closes it, the join — and the whole process — blocks forever.The fix
Add a bounded
Sync(timeout)overload:timeout→ a slow-but-alive stream is still drained in full (no truncation).Safe because the relay verdict is captured before the drain, so bounding it cannot change the reported result. Exec stdout/stderr workers become
std::optional<ScopedRelay>to share the same path. Defaultc_relayDrainTimeout = 60s.Why bound at teardown and not per-read: a per-read timeout would truncate a slow-but-alive stream; a teardown bound only trips when the stream truly never ends.
Testing
New
ScopedRelayunit tests:Sync(timeout)returns bounded (no hang).Plus a full MSI deployed to a live WSL host: install and exec (20k / 200k lines) complete cleanly, no hang.
@shuaiyuanxx — could you review? You own the relay / centralized-IO path. (Bug 62543726 assigned to you.)