From c8c6448f49733ae26b96ca118d0f70cfe8bf64d0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 10 May 2026 15:06:01 +0000 Subject: [PATCH 1/5] Initial plan From 3e0515c86b7f69e8ce8f6b6e73ee72258c402fc2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 10 May 2026 15:10:29 +0000 Subject: [PATCH 2/5] Fix lifetime callback cleanup for threadpool waits and timers Agent-Logs-Url: https://github.com/microsoft/WSL/sessions/b6855d2b-63ea-4edf-ba7c-81eb71f49268 Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- src/windows/service/exe/Lifetime.cpp | 39 +++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/src/windows/service/exe/Lifetime.cpp b/src/windows/service/exe/Lifetime.cpp index 54e1361bd..35f586ec9 100644 --- a/src/windows/service/exe/Lifetime.cpp +++ b/src/windows/service/exe/Lifetime.cpp @@ -58,6 +58,7 @@ void LifetimeManager::ClearCallbacks() // scope). std::vector callbacks; std::vector waits; + std::vector timers; { std::lock_guard lock(m_lock); @@ -66,8 +67,15 @@ void LifetimeManager::ClearCallbacks() for (auto& callback : m_callbackList) { + if (callback.timer) + { + callback.CancelTimer(); + timers.emplace_back(callback.timer.release()); + } + for (auto& child : callback.clientProcesses) { + SetThreadpoolWait(child.terminationWait.get(), nullptr, nullptr); waits.emplace_back(child.terminationWait.release()); } @@ -144,13 +152,32 @@ bool LifetimeManager::RemoveCallback(_In_ ULONG64 ClientKey) { bool callbackFound = false; ClientCallback oldClient{}; - std::lock_guard lock(m_lock); - const auto client = _FindClient(ClientKey); - if (client != m_callbackList.end()) + std::vector waits; + std::vector timers; { - oldClient = std::move(*client); - m_callbackList.erase(client); - callbackFound = true; + std::lock_guard lock(m_lock); + const auto client = _FindClient(ClientKey); + if (client != m_callbackList.end()) + { + oldClient = std::move(*client); + m_callbackList.erase(client); + callbackFound = true; + } + } + + if (callbackFound) + { + if (oldClient.timer) + { + oldClient.CancelTimer(); + timers.emplace_back(oldClient.timer.release()); + } + + for (auto& process : oldClient.clientProcesses) + { + SetThreadpoolWait(process.terminationWait.get(), nullptr, nullptr); + waits.emplace_back(process.terminationWait.release()); + } } return callbackFound; From c3edfe8f0563baede5ddff5dfa24075cbe880e90 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 10 May 2026 15:13:54 +0000 Subject: [PATCH 3/5] Address review feedback in lifetime timer cleanup Agent-Logs-Url: https://github.com/microsoft/WSL/sessions/b6855d2b-63ea-4edf-ba7c-81eb71f49268 Co-authored-by: benhillis <17727402+benhillis@users.noreply.github.com> --- src/windows/service/exe/Lifetime.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/windows/service/exe/Lifetime.cpp b/src/windows/service/exe/Lifetime.cpp index 35f586ec9..8c542e39e 100644 --- a/src/windows/service/exe/Lifetime.cpp +++ b/src/windows/service/exe/Lifetime.cpp @@ -69,7 +69,7 @@ void LifetimeManager::ClearCallbacks() { if (callback.timer) { - callback.CancelTimer(); + SetThreadpoolTimer(callback.timer.get(), nullptr, 0, 0); timers.emplace_back(callback.timer.release()); } @@ -169,7 +169,7 @@ bool LifetimeManager::RemoveCallback(_In_ ULONG64 ClientKey) { if (oldClient.timer) { - oldClient.CancelTimer(); + SetThreadpoolTimer(oldClient.timer.get(), nullptr, 0, 0); timers.emplace_back(oldClient.timer.release()); } From 2d3c409bc803ed5b9a4b1f20acc6c3ceeb6a39e4 Mon Sep 17 00:00:00 2001 From: Ben Hillis Date: Mon, 11 May 2026 09:31:46 -0700 Subject: [PATCH 4/5] Use Cancel{Wait,Timer} helpers; expand teardown rationale Replace open-coded SetThreadpoolWait/SetThreadpoolTimer null calls in ClearCallbacks and RemoveCallback with the existing ClientCallback::CancelTimer helper and a new symmetric OwnedProcess::CancelWait helper. Expand the ClearCallbacks comment block to explain the full strategy: why the per-client members use the _nowait wil variants (chain-of-waits in m_lastCallbackWait/m_lastTimerWait), why teardown re-homes them into the cancel-and-wait variants, and how in-flight callbacks are drained safely outside m_lock by the local container destructors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windows/service/exe/Lifetime.cpp | 36 ++++++++++++++++++++++------ src/windows/service/exe/Lifetime.h | 1 + 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/windows/service/exe/Lifetime.cpp b/src/windows/service/exe/Lifetime.cpp index 8c542e39e..22e8ed7d2 100644 --- a/src/windows/service/exe/Lifetime.cpp +++ b/src/windows/service/exe/Lifetime.cpp @@ -49,13 +49,24 @@ void LifetimeManager::ClearCallbacks() // (1) deadlocks // (2) concurrent modification of the callback list // (3) closing the process handle while the wait is still pending + // (4) leaking threadpool waits/timers that were never canceled // // The strategy is to: // 1. Take the lock - // 2. Move all clients to a local list + // 2. For each callback, cancel its wait/timer (so no further callbacks are + // queued) and re-home the handle into a regular (cancel + drain + close) + // wil container; move the rest of the callback into a local list // 3. Release the lock - // 4. Wait for any pending callbacks (when the local vectors go out of - // scope). + // 4. Let the local containers go out of scope, which drains any in-flight + // callbacks (each callback acquires m_lock - which we no longer hold, + // finds nothing in m_callbackList, and returns) and closes the + // threadpool objects + // + // The per-client members are the _nowait variants so the chain-of-waits in + // m_lastCallbackWait/m_lastTimerWait can swap them from inside their own + // callback without self-deadlocking. When they're being torn down outside + // a callback context (here and in RemoveCallback) we deliberately re-home + // them into the cancel-and-wait variants so teardown is fully synchronous. std::vector callbacks; std::vector waits; std::vector timers; @@ -69,13 +80,13 @@ void LifetimeManager::ClearCallbacks() { if (callback.timer) { - SetThreadpoolTimer(callback.timer.get(), nullptr, 0, 0); + callback.CancelTimer(); timers.emplace_back(callback.timer.release()); } for (auto& child : callback.clientProcesses) { - SetThreadpoolWait(child.terminationWait.get(), nullptr, nullptr); + child.CancelWait(); waits.emplace_back(child.terminationWait.release()); } @@ -165,17 +176,23 @@ bool LifetimeManager::RemoveCallback(_In_ ULONG64 ClientKey) } } + // The entry has been removed from m_callbackList so any callback that fires + // from here on will no-op. Cancel the per-client wait/timer (so no further + // callbacks are queued) and re-home the _nowait handles into cancel-and-wait + // wil containers; their destructors below drain any in-flight callbacks and + // close the threadpool objects, all outside m_lock. See ClearCallbacks() for + // the longer rationale. if (callbackFound) { if (oldClient.timer) { - SetThreadpoolTimer(oldClient.timer.get(), nullptr, 0, 0); + oldClient.CancelTimer(); timers.emplace_back(oldClient.timer.release()); } for (auto& process : oldClient.clientProcesses) { - SetThreadpoolWait(process.terminationWait.get(), nullptr, nullptr); + process.CancelWait(); waits.emplace_back(process.terminationWait.release()); } } @@ -325,6 +342,11 @@ void LifetimeManager::OwnedProcess::operator=(OwnedProcess&& source) noexcept terminationWait = std::move(source.terminationWait); } +void LifetimeManager::OwnedProcess::CancelWait() const +{ + SetThreadpoolWait(terminationWait.get(), nullptr, nullptr); +} + void LifetimeManager::OwnedProcess::InitializeListenForTermination(_In_ PTP_WAIT_CALLBACK Callback, _In_ PVOID Context) { terminationWait.reset(CreateThreadpoolWait(Callback, Context, nullptr)); diff --git a/src/windows/service/exe/Lifetime.h b/src/windows/service/exe/Lifetime.h index 159f62217..82d5038cc 100644 --- a/src/windows/service/exe/Lifetime.h +++ b/src/windows/service/exe/Lifetime.h @@ -45,6 +45,7 @@ class LifetimeManager OwnedProcess(const OwnedProcess&) = delete; void operator=(const OwnedProcess&) = delete; + void CancelWait() const; void InitializeListenForTermination(_In_ PTP_WAIT_CALLBACK Callback, _In_ PVOID Context); void ListenForTermination() const; From 7d1e3974d8bc963a8cb6a45a6b225e9413c03864 Mon Sep 17 00:00:00 2001 From: Ben Hillis Date: Mon, 11 May 2026 10:46:39 -0700 Subject: [PATCH 5/5] Make threadpool handle re-homing exception-safe Address Copilot reviewer feedback: release()-then-emplace_back leaks the raw PTP_WAIT/PTP_TIMER if the vector grows and allocation throws (the source has already lost ownership before emplace_back can construct the new element). Wrap each released handle in a named wil RAII temporary first, then push_back(std::move(...)). The temporary owns the handle while push_back may throw, ensuring the handle is properly closed via the destructor on failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windows/service/exe/Lifetime.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/windows/service/exe/Lifetime.cpp b/src/windows/service/exe/Lifetime.cpp index 22e8ed7d2..5e488c368 100644 --- a/src/windows/service/exe/Lifetime.cpp +++ b/src/windows/service/exe/Lifetime.cpp @@ -81,13 +81,15 @@ void LifetimeManager::ClearCallbacks() if (callback.timer) { callback.CancelTimer(); - timers.emplace_back(callback.timer.release()); + wil::unique_threadpool_timer scopedTimer{callback.timer.release()}; + timers.push_back(std::move(scopedTimer)); } for (auto& child : callback.clientProcesses) { child.CancelWait(); - waits.emplace_back(child.terminationWait.release()); + wil::unique_threadpool_wait scopedWait{child.terminationWait.release()}; + waits.push_back(std::move(scopedWait)); } callbacks.emplace_back(std::move(callback)); @@ -187,13 +189,15 @@ bool LifetimeManager::RemoveCallback(_In_ ULONG64 ClientKey) if (oldClient.timer) { oldClient.CancelTimer(); - timers.emplace_back(oldClient.timer.release()); + wil::unique_threadpool_timer scopedTimer{oldClient.timer.release()}; + timers.push_back(std::move(scopedTimer)); } for (auto& process : oldClient.clientProcesses) { process.CancelWait(); - waits.emplace_back(process.terminationWait.release()); + wil::unique_threadpool_wait scopedWait{process.terminationWait.release()}; + waits.push_back(std::move(scopedWait)); } }