diff --git a/esperanto-tools-libs/src/EventManager.cpp b/esperanto-tools-libs/src/EventManager.cpp index 4b70a6e01..1a0332c3d 100644 --- a/esperanto-tools-libs/src/EventManager.cpp +++ b/esperanto-tools-libs/src/EventManager.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -29,10 +30,24 @@ void EventManager::addOnDispatchCallback(OnDispatchCallback callback) { EventId EventManager::getNextId() { std::lock_guard lock(mutex_); - auto res = EventId{nextEventId_++}; - onflyEvents_.emplace(res); - RT_VLOG(LOW) << "Last event id: " << static_cast(res); - return res; + using EventIdValue = std::underlying_type_t; + constexpr auto kEventIdCount = static_cast(std::numeric_limits::max()) + 1; + + // EventId is a uint16_t and long-running inference processes legitimately + // wrap the counter. Never return an ID that is still in flight: emplace() + // used to fail silently here, so the newer command stole the older command's + // completion and the second response was reported as an unknown event. + for (size_t attempt = 0; attempt < kEventIdCount; ++attempt) { + auto res = EventId{nextEventId_++}; + if (onflyEvents_.find(res) != onflyEvents_.end() || blockedThreads_.find(res) != blockedThreads_.end()) { + continue; + } + onflyEvents_.emplace(res); + RT_VLOG(LOW) << "Last event id: " << static_cast(res); + return res; + } + + throw Exception("No free runtime event IDs; synchronize the stream before submitting more commands"); } void EventManager::dispatch(EventId event) { @@ -115,4 +130,4 @@ EventManager::~EventManager() { << static_cast(event); cv->notify_all(); } -} \ No newline at end of file +} diff --git a/esperanto-tools-libs/src/RuntimeImp.cpp b/esperanto-tools-libs/src/RuntimeImp.cpp index b8b84f154..b9cf452f6 100644 --- a/esperanto-tools-libs/src/RuntimeImp.cpp +++ b/esperanto-tools-libs/src/RuntimeImp.cpp @@ -869,8 +869,16 @@ void RuntimeImp::dispatch(EventId event) { evt.setEvent(event); getProfiler()->record(evt); streamManager_.removeEvent(event); + // Keep the ID in flight until observers have consumed the completion. This + // prevents a wrapped ID from being reused between EventManager::dispatch() + // and the server worker forwarding the old completion to its client. + try { + notify(event); + } catch (...) { + eventManager_.dispatch(event); + throw; + } eventManager_.dispatch(event); - notify(event); } void RuntimeImp::checkDevice(DeviceId device) { diff --git a/esperanto-tools-libs/tests/unit-tests/test_event_manager.cpp b/esperanto-tools-libs/tests/unit-tests/test_event_manager.cpp index 3c256374a..0a2899ffa 100644 --- a/esperanto-tools-libs/tests/unit-tests/test_event_manager.cpp +++ b/esperanto-tools-libs/tests/unit-tests/test_event_manager.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #pragma GCC diagnostic push @@ -106,6 +107,37 @@ TEST_F(EventManagerF, severalEvents) { } } +TEST_F(EventManagerF, wrapSkipsEventIdsStillInFlight) { + using EventIdValue = std::underlying_type_t; + constexpr auto maxId = std::numeric_limits::max(); + + em_.nextEventId_ = maxId; + auto atMax = em_.getNextId(); + auto atZero = em_.getNextId(); + EXPECT_EQ(static_cast(atMax), maxId); + EXPECT_EQ(static_cast(atZero), 0); + + em_.nextEventId_ = maxId; + auto skippedToOne = em_.getNextId(); + EXPECT_EQ(static_cast(skippedToOne), 1); + + em_.dispatch(atMax); + em_.dispatch(atZero); + em_.dispatch(skippedToOne); +} + +TEST_F(EventManagerF, exhaustionFailsInsteadOfReturningDuplicateId) { + using EventIdValue = std::underlying_type_t; + constexpr auto eventIdCount = + static_cast(std::numeric_limits::max()) + 1; + + for (size_t i = 0; i < eventIdCount; ++i) { + em_.onflyEvents_.emplace(EventId{static_cast(i)}); + } + EXPECT_THROW(em_.getNextId(), Exception); + em_.onflyEvents_.clear(); +} + TEST_F(EventManagerF, blockingThreads) { auto nEvents = 10U; auto nThreads = 10000U;