From ff7385054181bb4b119e762cde4d44b471491b3e Mon Sep 17 00:00:00 2001 From: exzile Date: Sat, 27 Jun 2026 19:40:13 -0400 Subject: [PATCH 1/2] Release infer-request slot if start_async() throws in modelInferAsync In the async inference path (modelInferAsync, used by KServe gRPC and the C-API async endpoint), the ExecutingStreamIdGuard that holds an infer-request pool slot is std::move-d into the OV completion-callback lambda before inferRequest.start_async() is called. The slot is returned to OVInferRequestsQueue only when that lambda is destroyed, which normally happens when the async completion callback fires. If start_async() throws, the function returns immediately and the completion callback never fires, so the lambda (sole owner of the guard) is never destroyed and the infer-request slot is never returned to the pool. Under long-running concurrent load, repeated start_async failures slowly drain the pool until no idle infer request is available; requests then block and gRPC surfaces ResourceExhausted even though CPU/GPU/RAM are not saturated (matches the symptom class in #2871). Clear the callback in both start_async() catch blocks so the captured guard is destroyed and the slot is returned, mirroring the existing "set empty callback to release captures" pattern used after normal completion. The synchronous infer() path is unaffected (it uses a stack-allocated guard with RAII release on all paths). Builds clean (//src:ovms //src:ovms_test); CAPIInference.AsyncErrorHandling passes. The change is confined to the start_async error path. Signed-off-by: exzile --- src/inference_executor.hpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/inference_executor.hpp b/src/inference_executor.hpp index 1092c82f7c..9804f1c145 100644 --- a/src/inference_executor.hpp +++ b/src/inference_executor.hpp @@ -207,9 +207,17 @@ Status modelInferAsync(ModelInstance& instance, const RequestType* request, inferRequest.start_async(); } catch (std::exception& e) { SPDLOG_DEBUG("caught exception in ov::InferRequest.start_async: {}", e.what()); + // start_async threw, so the completion callback will never fire to release the + // captured stream-id guard; clear the callback to destroy it and return the + // infer-request slot to the pool (otherwise the slot leaks -> #2871). + inferRequest.set_callback([](std::exception_ptr) {}); return StatusCode::OV_INTERNAL_INFERENCE_ERROR; } catch (...) { SPDLOG_DEBUG("caught exception in ov::InferRequest.start_async"); + // start_async threw, so the completion callback will never fire to release the + // captured stream-id guard; clear the callback to destroy it and return the + // infer-request slot to the pool (otherwise the slot leaks -> #2871). + inferRequest.set_callback([](std::exception_ptr) {}); return StatusCode::OV_INTERNAL_INFERENCE_ERROR; } return StatusCode::OK; From 69704b4e4de4f17a37968803fbaad82706ba7f1e Mon Sep 17 00:00:00 2001 From: exzile Date: Sat, 27 Jun 2026 22:02:32 -0400 Subject: [PATCH 2/2] Fix deadlock on start_async error path and add regression test The previous commit cleared the infer-request callback on the start_async() error path to release the leaked pool slot. That introduced a latent crash: clearing the callback destroys the completion lambda while OpenVINO holds the InferRequest's internal lock, and that lambda owns an OutputKeeper whose destructor calls request.set_tensor() back into the same InferRequest. For models that support output-tensor reset (the common case) this reenters the locked request and throws "resource deadlock would occur" out of a noexcept destructor, terminating the server - on the very error path the fix targets. Fix it without reintroducing the leak: - OutputKeeper gains cancel(), which disables the destructor's tensor restore. - modelInferAsync captures the OutputKeeper into the callback by copy and keeps the original handle in function scope, so clearing the callback no longer destroys the OutputKeeper under the lock. - The error path now cancels the OutputKeeper before clearing the callback (inference never ran, so there is nothing to restore) and the retained handle's later destruction is a no-op. Both catch blocks share a helper, releaseInferRequestSlotAfterStartAsyncFailure(). Add a regression test (CAPIInference.AsyncStartAsyncThrowReleasesInferRequestSlot) that forces start_async() to throw via a small virtual seam (ModelInstance::startAsyncInference) on a single-stream pool and asserts the slot is returned afterwards. The test reproduces both the original leak and the deadlock crash, and passes only with this fix. Existing async tests (AsyncWithCallbackDummy, AsyncErrorHandling) and the full CAPIInference suite (17/17) still pass. Signed-off-by: exzile Co-Authored-By: Claude Opus 4.8 --- src/inference_executor.hpp | 34 ++++++++++++++++------- src/modelinstance.cpp | 4 +++ src/modelinstance.hpp | 10 +++++++ src/outputkeeper.cpp | 3 ++ src/outputkeeper.hpp | 5 ++++ src/test/c_api_tests.cpp | 57 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 103 insertions(+), 10 deletions(-) diff --git a/src/inference_executor.hpp b/src/inference_executor.hpp index 9804f1c145..8c16fa7357 100644 --- a/src/inference_executor.hpp +++ b/src/inference_executor.hpp @@ -48,6 +48,26 @@ enum : unsigned int { POSTPROCESS, TIMER_END }; + +// Releases the infer-request slot acquired by modelInferAsync when start_async() throws. +// +// The stream-id guard that owns the slot was moved into the completion-callback lambda; +// since start_async() failed, that callback will never fire to destroy the lambda, so the +// slot would leak (#2871). Clearing the callback destroys the lambda and returns the slot. +// +// The callback lambda also holds a (copied) OutputKeeper. Clearing the callback runs while +// OpenVINO holds the InferRequest's internal lock, so the lambda's OutputKeeper copy must not +// reset output tensors there - doing so would reenter this InferRequest and deadlock +// (std::terminate via an exception leaving the noexcept destructor). The caller retains the +// original OutputKeeper handle (capture-by-copy), so we cancel it first: inference never ran, +// so there is nothing to restore, and the retained handle's destructor then becomes a no-op. +inline void releaseInferRequestSlotAfterStartAsyncFailure(ov::InferRequest& inferRequest, const std::shared_ptr& outKeeper) { + if (outKeeper) { + outKeeper->cancel(); + } + inferRequest.set_callback([](std::exception_ptr) {}); +} + template Status modelInferAsync(ModelInstance& instance, const RequestType* request, std::unique_ptr& modelUnloadGuardPtr) { @@ -136,7 +156,7 @@ Status modelInferAsync(ModelInstance& instance, const RequestType* request, { // order is important here - destructors are called in order from right to left inferRequest.set_callback( - [&instance, request, &inferRequest, userCallback, userCallbackData, modelUnloadGuardPtrMoved = std::shared_ptr(std::move(modelUnloadGuardPtr)), streamIdGuardMoved = std::move(executingStreamIdGuard), movedOutputKeeper = std::move(outKeeper)](std::exception_ptr exception) mutable { + [&instance, request, &inferRequest, userCallback, userCallbackData, modelUnloadGuardPtrMoved = std::shared_ptr(std::move(modelUnloadGuardPtr)), streamIdGuardMoved = std::move(executingStreamIdGuard), movedOutputKeeper = outKeeper](std::exception_ptr exception) mutable { struct CallbackGuard { OVMS_InferenceRequestCompletionCallback_t userCallback{nullptr}; void* userCallbackData{nullptr}; @@ -204,20 +224,14 @@ Status modelInferAsync(ModelInstance& instance, const RequestType* request, try { SPDLOG_DEBUG("ov::InferRequest: {}, inferRequest.start_async()", reinterpret_cast(&inferRequest)); - inferRequest.start_async(); + instance.startAsyncInference(inferRequest); } catch (std::exception& e) { SPDLOG_DEBUG("caught exception in ov::InferRequest.start_async: {}", e.what()); - // start_async threw, so the completion callback will never fire to release the - // captured stream-id guard; clear the callback to destroy it and return the - // infer-request slot to the pool (otherwise the slot leaks -> #2871). - inferRequest.set_callback([](std::exception_ptr) {}); + releaseInferRequestSlotAfterStartAsyncFailure(inferRequest, outKeeper); return StatusCode::OV_INTERNAL_INFERENCE_ERROR; } catch (...) { SPDLOG_DEBUG("caught exception in ov::InferRequest.start_async"); - // start_async threw, so the completion callback will never fire to release the - // captured stream-id guard; clear the callback to destroy it and return the - // infer-request slot to the pool (otherwise the slot leaks -> #2871). - inferRequest.set_callback([](std::exception_ptr) {}); + releaseInferRequestSlotAfterStartAsyncFailure(inferRequest, outKeeper); return StatusCode::OV_INTERNAL_INFERENCE_ERROR; } return StatusCode::OK; diff --git a/src/modelinstance.cpp b/src/modelinstance.cpp index 438b902b5e..d747beedc2 100644 --- a/src/modelinstance.cpp +++ b/src/modelinstance.cpp @@ -1604,6 +1604,10 @@ OVInferRequestsQueue& ModelInstance::getInferRequestsQueue() { return *inferRequestsQueue; } +void ModelInstance::startAsyncInference(ov::InferRequest& inferRequest) { + inferRequest.start_async(); +} + const size_t ModelInstance::getBatchSizeIndex() const { const auto& inputItr = this->inputsInfo.cbegin(); if (inputItr == this->inputsInfo.cend()) { diff --git a/src/modelinstance.hpp b/src/modelinstance.hpp index 76941e480c..63bfe07cee 100644 --- a/src/modelinstance.hpp +++ b/src/modelinstance.hpp @@ -533,6 +533,16 @@ class ModelInstance : public Servable { */ OVInferRequestsQueue& getInferRequestsQueue(); + /** + * @brief Starts asynchronous inference on the given infer request. + * + * Thin virtual wrapper over ov::InferRequest::start_async. In production it + * forwards unconditionally; it exists as a seam so tests can simulate + * start_async() throwing and verify the infer-request slot is released on + * that error path (see modelInferAsync and #2871). + */ + virtual void startAsyncInference(ov::InferRequest& inferRequest); + /** * @brief Combines plugin config from user with default config calculated at runtime * diff --git a/src/outputkeeper.cpp b/src/outputkeeper.cpp index f81a1de19b..886878027b 100644 --- a/src/outputkeeper.cpp +++ b/src/outputkeeper.cpp @@ -35,6 +35,9 @@ OutputKeeper::OutputKeeper(ov::InferRequest& request, const tensor_map_t& output } } OutputKeeper::~OutputKeeper() { + if (cancelled) { + return; + } for (auto [name, v] : outputs) { OV_LOGGER("ov::InferRequest: {}, request.set_tensor({}, {})", reinterpret_cast(&request), name, reinterpret_cast(&v)); request.set_tensor(name, v); diff --git a/src/outputkeeper.hpp b/src/outputkeeper.hpp index c755f3a070..87d79bfadc 100644 --- a/src/outputkeeper.hpp +++ b/src/outputkeeper.hpp @@ -27,7 +27,12 @@ namespace ovms { struct OutputKeeper { std::unordered_map outputs; ov::InferRequest& request; + bool cancelled{false}; OutputKeeper(ov::InferRequest& request, const tensor_map_t& outputsInfo); ~OutputKeeper(); + // Disable the output-tensor restore performed by the destructor. Used on the async + // start_async() error path, where inference never ran (so there is nothing to restore) + // and reentering the InferRequest from the destructor would deadlock (#2871). + void cancel() { this->cancelled = true; } }; } // namespace ovms diff --git a/src/test/c_api_tests.cpp b/src/test/c_api_tests.cpp index e59f27cbf4..0573eeba73 100644 --- a/src/test/c_api_tests.cpp +++ b/src/test/c_api_tests.cpp @@ -29,6 +29,7 @@ #include "../capi_frontend/capi_request_utils.hpp" #include "../deserialization_main.hpp" #include "../inference_executor.hpp" +#include "../ovinferrequestsqueue.hpp" #include "../capi_frontend/capi_utils.hpp" #include "../capi_frontend/serialization.hpp" #include "../capi_frontend/deserialization.hpp" @@ -2046,3 +2047,59 @@ TEST_F(CAPIInference, AsyncErrorHandling) { unloadGuard.reset(); instance.retireModel(); } + +// Model instance whose async inference start deterministically throws, to exercise the +// modelInferAsync start_async() error path (see #2871). Production ModelInstance forwards +// startAsyncInference() to ov::InferRequest::start_async(); here we override it to throw. +class MockModelInstanceThrowingOnStartAsync : public ovms::ModelInstance { +public: + MockModelInstanceThrowingOnStartAsync(ov::Core& ieCore) : + ModelInstance(std::string("UNUSED_NAME"), 0, ieCore, nullptr, nullptr) { + status = ovms::ModelVersionStatus("UNUSED_NAME", UNUSED_MODEL_VERSION, ovms::ModelVersionState::START); + } + virtual ~MockModelInstanceThrowingOnStartAsync() {} + ovms::Status loadModel(const ovms::ModelConfig& config) override { + return ModelInstance::loadModel(config); + } + void startAsyncInference(ov::InferRequest& inferRequest) override { + throw std::runtime_error("simulated start_async failure"); + } +}; + +// Regression test for #2871: when start_async() throws, modelInferAsync must release the +// infer-request slot it acquired. With a single-stream pool, a leak makes the slot +// permanently unavailable; we assert it is returned by querying the queue afterwards. +TEST_F(CAPIInference, AsyncStartAsyncThrowReleasesInferRequestSlot) { + ov::Core core; + MockModelInstanceThrowingOnStartAsync instance(core); + ovms::ModelConfig config = DUMMY_MODEL_CONFIG; + config.setNireq(1); // single slot -> a leaked slot is observable + ASSERT_EQ(instance.loadModel(config), ovms::StatusCode::OK); + std::unique_ptr unloadGuard; + instance.waitForLoaded(0, unloadGuard); + + // Sanity: the single slot is idle before inference. + { + auto idle = instance.getInferRequestsQueue().tryToGetIdleStream(); + ASSERT_TRUE(idle.has_value()); + instance.getInferRequestsQueue().returnStream(idle.value()); + } + + ovms::InferenceRequest request("dummy", 0); + std::vector in(DUMMY_MODEL_INPUT_SIZE, INITIAL_VALUE); + request.addInput(DUMMY_MODEL_INPUT_NAME, OVMS_DATATYPE_FP32, DUMMY_MODEL_SHAPE.data(), DUMMY_MODEL_SHAPE.size()); + request.setInputBuffer(DUMMY_MODEL_INPUT_NAME, in.data(), DUMMY_MODEL_INPUT_SIZE * sizeof(float), OVMS_BUFFERTYPE_CPU, 0); + CallbackUnblockingAndCheckingStruct callbackStruct; + request.setCompletionCallback(callbackCheckingIfErrorReported, &callbackStruct); + + auto status = ovms::modelInferAsync(instance, &request, unloadGuard); + EXPECT_EQ(status, ovms::StatusCode::OV_INTERNAL_INFERENCE_ERROR) << status.string(); + + // The slot acquired by modelInferAsync must have been returned to the pool despite the + // start_async() failure. Without the fix it leaks and tryToGetIdleStream() returns nullopt. + auto idleAfter = instance.getInferRequestsQueue().tryToGetIdleStream(); + EXPECT_TRUE(idleAfter.has_value()) << "infer-request slot leaked after start_async() threw (#2871)"; + + unloadGuard.reset(); + instance.retireModel(); +}