From bdb700ec56bbb34adcd348c3a407d7f9fd32ea71 Mon Sep 17 00:00:00 2001 From: BlackMark Date: Wed, 24 Jun 2026 01:12:29 +0200 Subject: [PATCH] Recover the video mirror from a WAIT_ABANDONED keyed mutex SharedTextureSource::update() acquired the shared-texture keyed mutex with `AcquireSync(...) == S_OK`. But WAIT_ABANDONED -- a prior owner (e.g. a host that crashed mid-acquire, then reconnected) died holding it -- actually GRANTS us ownership. Treating it as failure skipped the copy AND never released, so the next AcquireSync blocked forever and the mirror froze permanently after a crash + reconnect (directly relevant to the new reconnect path). Factor the decision into keyed_mutex_acquired(HRESULT) (capture/keyed_mutex.hpp): S_OK or WAIT_ABANDONED -> copy + release; timeout/hard errors -> skip the frame. update() now uses it. Test-first: keyed_mutex_test asserts WAIT_ABANDONED is treated as acquired while the genuine "didn't get it" cases (timeout, E_FAIL, device-removed) are not. The full cross-process abandonment is keyed-mutex OS semantics, not re-tested with a child process here -- the predicate is the regression surface. Co-Authored-By: Claude Opus 4.8 --- README.md | 4 --- host/src/capture/keyed_mutex.hpp | 21 +++++++++++++++ host/src/capture/shared_texture.cpp | 7 +++-- tests/CMakeLists.txt | 7 +++++ tests/keyed_mutex_test.cpp | 42 +++++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 6 deletions(-) create mode 100644 host/src/capture/keyed_mutex.hpp create mode 100644 tests/keyed_mutex_test.cpp diff --git a/README.md b/README.md index e9bfd29..187ee9f 100644 --- a/README.md +++ b/README.md @@ -113,10 +113,6 @@ default** and covers anything the hooked path doesn't. From an in-depth review pass. Each item is fixed test-first (a failing test, then the fix) and lands as its own commit; "verify" items are confirmed real before any change, and dropped if not. -Confirmed bugs: -- **Keyed-mutex `WAIT_ABANDONED` not handled** — `shared_texture` treats it as "skip", so a mirror - never recovers after a host crash + reconnect. Treat it as acquired (copy + release). - Correctness (verify, then fix if real): - **`vk_hook` / `vk_layer` `g_swaps`** — no synchronization on push/iterate and never pruned on swapchain destroy (unbounded growth + stale-handle match). Add a guard + a `vkDestroySwapchainKHR` diff --git a/host/src/capture/keyed_mutex.hpp b/host/src/capture/keyed_mutex.hpp new file mode 100644 index 0000000..a89942b --- /dev/null +++ b/host/src/capture/keyed_mutex.hpp @@ -0,0 +1,21 @@ +// The acquire decision for the shared video texture's keyed mutex, factored out so it can be tested +// without a D3D device (capture/shared_texture.cpp uses it). +#pragma once + +#include // HRESULT, S_OK, WAIT_ABANDONED, WAIT_TIMEOUT + +namespace coop +{ + +// True when an IDXGIKeyedMutex::AcquireSync result means we now HOLD the mutex and must copy + then +// release it. S_OK is the normal case. WAIT_ABANDONED is success-with-recovery: a previous owner +// (e.g. a host that crashed while holding it, then reconnected) terminated without releasing, so the +// OS hands ownership to us -- treating it as a failure would skip the copy AND leak ownership, so the +// next AcquireSync would block forever and the mirror would freeze. Timeout / hard errors mean "we +// didn't get it, skip this frame." +inline bool keyed_mutex_acquired(HRESULT acquire_result) +{ + return acquire_result == S_OK || acquire_result == static_cast(WAIT_ABANDONED); +} + +} // namespace coop diff --git a/host/src/capture/shared_texture.cpp b/host/src/capture/shared_texture.cpp index 1371226..d6dd03b 100644 --- a/host/src/capture/shared_texture.cpp +++ b/host/src/capture/shared_texture.cpp @@ -1,6 +1,7 @@ #include "capture/shared_texture.hpp" #include "capture/dxgi_format.hpp" +#include "capture/keyed_mutex.hpp" #include "coop/protocol.hpp" #include "coop/shared_memory.hpp" @@ -194,8 +195,10 @@ bool SharedTextureSource::update(const VideoShareView& share, unsigned long pid) return srv_ != nullptr; // no new frame; keep showing the last copy } - // Bounded wait so a stalled producer can't hang the host's render thread. - if (mutex_->AcquireSync(kVideoMutexKey, 8) == S_OK) + // Bounded wait so a stalled producer can't hang the host's render thread. WAIT_ABANDONED (a prior + // owner died holding the mutex -- e.g. a host that crashed and reconnected) counts as acquired: + // recover by copying + releasing rather than skipping, which would hold it forever and freeze. + if (keyed_mutex_acquired(mutex_->AcquireSync(kVideoMutexKey, 8))) { ctx_->CopyResource(private_.Get(), shared_.Get()); mutex_->ReleaseSync(kVideoMutexKey); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9ac302b..33665e9 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -173,6 +173,12 @@ add_executable(srgb_format_test srgb_format_test.cpp) target_include_directories(srgb_format_test PRIVATE ${CMAKE_SOURCE_DIR}/host/src) add_test(NAME srgb_format_test COMMAND srgb_format_test) +# Unit test for the keyed-mutex acquire decision (capture/keyed_mutex.hpp): WAIT_ABANDONED must be +# treated as "acquired" so a mirror recovers after a host crash instead of freezing. Header-only. +add_executable(keyed_mutex_test keyed_mutex_test.cpp) +target_include_directories(keyed_mutex_test PRIVATE ${CMAKE_SOURCE_DIR}/host/src) +add_test(NAME keyed_mutex_test COMMAND keyed_mutex_test) + # In-process self-test for the Present-hook video path. Reuses the shipping # present_hook.cpp and drives a real D3D11 swapchain in the same process, so it # exercises the IDXGISwapChain::Present inline hook, the shared keyed-mutex @@ -311,6 +317,7 @@ coop_output_subdir(tests dinput_hook_test audio_ring_test ipc_server_test + keyed_mutex_test detour_gate_test hook_install_test mkb_ring_test diff --git a/tests/keyed_mutex_test.cpp b/tests/keyed_mutex_test.cpp new file mode 100644 index 0000000..a32c31e --- /dev/null +++ b/tests/keyed_mutex_test.cpp @@ -0,0 +1,42 @@ +// Unit test for the keyed-mutex acquire decision behind SharedTextureSource::update() +// (host/src/capture/keyed_mutex.hpp). The bug: AcquireSync returning WAIT_ABANDONED -- a prior owner +// (e.g. a host that crashed mid-acquire) died holding the mutex, which GRANTS us ownership -- was +// treated as failure, so the host skipped the frame AND never released, freezing the mirror forever. +// The predicate must treat WAIT_ABANDONED as "acquired" (copy + release) while still rejecting the +// real "didn't get it" cases. Deterministic; no D3D device. +#include + +#include + +#include + +#include "capture/keyed_mutex.hpp" + +using namespace coop; + +namespace +{ +int g_failures = 0; +void check(bool ok, const char* what) +{ + std::printf("%s %s\n", ok ? " ok:" : "FAIL:", what); + if (!ok) + { + ++g_failures; + } +} +} // namespace + +int main() +{ + check(keyed_mutex_acquired(S_OK), "S_OK -> acquired (copy + release)"); + check(keyed_mutex_acquired(static_cast(WAIT_ABANDONED)), + "WAIT_ABANDONED -> acquired (recover: prior owner died holding it; we own it now)"); + check(!keyed_mutex_acquired(static_cast(WAIT_TIMEOUT)), + "WAIT_TIMEOUT -> not acquired (producer busy; skip this frame)"); + check(!keyed_mutex_acquired(E_FAIL), "E_FAIL -> not acquired"); + check(!keyed_mutex_acquired(DXGI_ERROR_DEVICE_REMOVED), "DEVICE_REMOVED -> not acquired"); + + std::printf(g_failures == 0 ? "PASS keyed_mutex_test\n" : "FAILED keyed_mutex_test (%d)\n", g_failures); + return g_failures == 0 ? 0 : 1; +}