From 5954876f8d03a6f79643b12cea170b68b69a03a0 Mon Sep 17 00:00:00 2001 From: Sasa Mudri Date: Mon, 25 May 2026 11:22:49 +0200 Subject: [PATCH 1/3] Fix ServiceContext members lifetime & update usage of 'm_isShuttingDown'. --- serverManager/common/include/ISessionServerAppManager.h | 1 + serverManager/common/source/SessionServerAppManager.cpp | 6 +++++- serverManager/common/source/SessionServerAppManager.h | 4 +++- serverManager/service/include/ServiceContext.h | 2 +- serverManager/service/source/ServiceContext.cpp | 8 ++++++++ .../serverManager/mocks/SessionServerAppManagerMock.h | 1 + 6 files changed, 19 insertions(+), 3 deletions(-) diff --git a/serverManager/common/include/ISessionServerAppManager.h b/serverManager/common/include/ISessionServerAppManager.h index 73011eaf7..c9062d9e9 100644 --- a/serverManager/common/include/ISessionServerAppManager.h +++ b/serverManager/common/include/ISessionServerAppManager.h @@ -52,6 +52,7 @@ class ISessionServerAppManager virtual bool setLogLevels(const service::LoggingLevels &logLevels) const = 0; virtual void restartServer(int serverId) = 0; virtual void onServerStartupTimeout(int serverId) = 0; + virtual void setShuttingDown() = 0; }; } // namespace rialto::servermanager::common diff --git a/serverManager/common/source/SessionServerAppManager.cpp b/serverManager/common/source/SessionServerAppManager.cpp index 08b465973..5b6cee1f7 100644 --- a/serverManager/common/source/SessionServerAppManager.cpp +++ b/serverManager/common/source/SessionServerAppManager.cpp @@ -47,6 +47,11 @@ SessionServerAppManager::~SessionServerAppManager() m_eventThread.reset(); } +void SessionServerAppManager::setShuttingDown() +{ + m_isShuttingDown = true; +} + void SessionServerAppManager::preloadSessionServers(unsigned numOfPreloadedServers) { m_eventThread->add( @@ -392,7 +397,6 @@ void SessionServerAppManager::handleAck(int serverId, int pingId, bool success) void SessionServerAppManager::shutdownAllSessionServers() { - m_isShuttingDown = true; m_healthcheckService.reset(); for (const auto &kSessionServer : m_sessionServerApps) { diff --git a/serverManager/common/source/SessionServerAppManager.h b/serverManager/common/source/SessionServerAppManager.h index e16f26f70..dbfcaa037 100644 --- a/serverManager/common/source/SessionServerAppManager.h +++ b/serverManager/common/source/SessionServerAppManager.h @@ -29,6 +29,7 @@ #include "ISessionServerAppManager.h" #include "IStateObserver.h" #include "SessionServerAppFactory.h" +#include #include #include #include @@ -62,6 +63,7 @@ class SessionServerAppManager : public ISessionServerAppManager bool setLogLevels(const service::LoggingLevels &logLevels) const override; void restartServer(int serverId) override; void onServerStartupTimeout(int serverId) override; + void setShuttingDown() override; private: bool connectSessionServer(const std::shared_ptr &sessionServer); @@ -99,7 +101,7 @@ class SessionServerAppManager : public ISessionServerAppManager std::shared_ptr m_stateObserver; std::unique_ptr m_healthcheckService; const firebolt::rialto::ipc::INamedSocketFactory &m_namedSocketFactory; - bool m_isShuttingDown; + std::atomic_bool m_isShuttingDown; }; } // namespace rialto::servermanager::common diff --git a/serverManager/service/include/ServiceContext.h b/serverManager/service/include/ServiceContext.h index 746e1193b..80c97e61b 100644 --- a/serverManager/service/include/ServiceContext.h +++ b/serverManager/service/include/ServiceContext.h @@ -38,7 +38,7 @@ class ServiceContext : public IServiceContext std::chrono::milliseconds sessionServerStartupTimeout, std::chrono::seconds healthcheckInterval, unsigned numOfFailedPingsBeforeRecovery, unsigned int socketPermissions, const std::string &socketOwner, const std::string &socketGroup); - virtual ~ServiceContext() = default; + ~ServiceContext() override; common::ISessionServerAppManager &getSessionServerAppManager() override; diff --git a/serverManager/service/source/ServiceContext.cpp b/serverManager/service/source/ServiceContext.cpp index 8b65cb75a..2a3c7d865 100644 --- a/serverManager/service/source/ServiceContext.cpp +++ b/serverManager/service/source/ServiceContext.cpp @@ -38,6 +38,14 @@ ServiceContext::ServiceContext(const std::shared_ptr &stateObser { } +ServiceContext::~ServiceContext() +{ + if (m_sessionServerAppManager) + { + m_sessionServerAppManager->setShuttingDown(); + } +} + common::ISessionServerAppManager &ServiceContext::getSessionServerAppManager() { return *m_sessionServerAppManager; diff --git a/tests/unittests/serverManager/mocks/SessionServerAppManagerMock.h b/tests/unittests/serverManager/mocks/SessionServerAppManagerMock.h index de41777ab..e4f65acd5 100644 --- a/tests/unittests/serverManager/mocks/SessionServerAppManagerMock.h +++ b/tests/unittests/serverManager/mocks/SessionServerAppManagerMock.h @@ -47,6 +47,7 @@ class SessionServerAppManagerMock : public ISessionServerAppManager MOCK_METHOD(bool, setLogLevels, (const service::LoggingLevels &), (const, override)); MOCK_METHOD(void, restartServer, (int serverId), (override)); MOCK_METHOD(void, onServerStartupTimeout, (int serverId), (override)); + MOCK_METHOD(void, setShuttingDown, (), (override)); }; } // namespace rialto::servermanager::common From d3f07c91f4921fe2e96c877d1c9c1b936e16d963 Mon Sep 17 00:00:00 2001 From: Sasa Mudri Date: Wed, 27 May 2026 14:59:51 +0200 Subject: [PATCH 2/3] UTs added. --- .../common/SessionServerAppManagerTests.cpp | 6 ++++++ .../common/SessionServerAppManagerTestsFixture.cpp | 5 +++++ .../common/SessionServerAppManagerTestsFixture.h | 1 + .../unittests/service/ServerManagerServiceTests.cpp | 12 ++++++++++++ 4 files changed, 24 insertions(+) diff --git a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp index 63353c5aa..cf426e201 100644 --- a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp +++ b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp @@ -319,6 +319,12 @@ TEST_F(SessionServerAppManagerTests, SessionServerShouldSkipRestart) sessionServerWillKillRunningApplication(); } +TEST_F(SessionServerAppManagerTests, SessionServerShouldSkipRestartWhenShuttingDown) +{ + triggerSetShuttingDown(); + triggerRestartServer(); +} + TEST_F(SessionServerAppManagerTests, SessionServerShouldReportStartupTimeout) { sessionServerWillLaunch(firebolt::rialto::common::SessionServerState::INACTIVE); diff --git a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.cpp b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.cpp index 83994a266..906d83901 100644 --- a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.cpp +++ b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.cpp @@ -539,3 +539,8 @@ void SessionServerAppManagerTests::triggerOnServerStartupTimeout() { m_sut->onServerStartupTimeout(kServerId); } + +void SessionServerAppManagerTests::triggerSetShuttingDown() +{ + m_sut->setShuttingDown(); +} diff --git a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.h b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.h index 5dfa7f42a..5ffb60a7f 100644 --- a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.h +++ b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.h @@ -88,6 +88,7 @@ class SessionServerAppManagerTests : public testing::Test void triggerSendPingEvents(); void triggerRestartServer(); void triggerOnServerStartupTimeout(); + void triggerSetShuttingDown(); private: std::unique_ptr m_controller; diff --git a/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp b/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp index 6ed08bc8b..e509a452a 100644 --- a/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp +++ b/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp @@ -19,6 +19,7 @@ #include "RialtoLogging.h" #include "ServerManagerServiceTestsFixture.h" +#include "ServiceContext.h" #include "gtest/gtest.h" namespace @@ -27,6 +28,12 @@ const std::string kAppName{"YouTube"}; const firebolt::rialto::common::SessionServerState kAppState{firebolt::rialto::common::SessionServerState::INACTIVE}; const std::string kAppSocket{getenv("RIALTO_SOCKET_PATH")}; const firebolt::rialto::common::AppConfig kAppConfig{kAppSocket}; + +void createAndDestroyServiceContext() +{ + rialto::servermanager::service::ServiceContext + context{nullptr, {}, "", std::chrono::milliseconds{0}, std::chrono::seconds{0}, 0, 0, "", ""}; +} } // namespace TEST_F(ServerManagerServiceTests, initiateApplicationShouldReturnTrueIfOperationSucceeded) @@ -82,3 +89,8 @@ TEST_F(ServerManagerServiceTests, registerLogHandlerShouldFailWhenPtrIsNull) { EXPECT_FALSE(triggerRegisterLogHandler(nullptr)); } + +TEST(ServiceContextTests, DestructorShouldMarkSessionServerAppManagerAsShuttingDown) +{ + ASSERT_NO_THROW(createAndDestroyServiceContext()); +} From 928f41444d1f48c5618da0b38ac69beeb80e6a98 Mon Sep 17 00:00:00 2001 From: Sasa Mudri Date: Wed, 27 May 2026 16:22:49 +0200 Subject: [PATCH 3/3] Resolve copilot review comments. --- serverManager/common/source/SessionServerAppManager.cpp | 1 + serverManager/service/include/ServiceContext.h | 1 + .../unittests/common/SessionServerAppManagerTests.cpp | 3 +++ .../unittests/service/ServerManagerServiceTests.cpp | 2 +- 4 files changed, 6 insertions(+), 1 deletion(-) diff --git a/serverManager/common/source/SessionServerAppManager.cpp b/serverManager/common/source/SessionServerAppManager.cpp index 5b6cee1f7..fada4f26f 100644 --- a/serverManager/common/source/SessionServerAppManager.cpp +++ b/serverManager/common/source/SessionServerAppManager.cpp @@ -397,6 +397,7 @@ void SessionServerAppManager::handleAck(int serverId, int pingId, bool success) void SessionServerAppManager::shutdownAllSessionServers() { + setShuttingDown(); m_healthcheckService.reset(); for (const auto &kSessionServer : m_sessionServerApps) { diff --git a/serverManager/service/include/ServiceContext.h b/serverManager/service/include/ServiceContext.h index 80c97e61b..cf4f6ba6b 100644 --- a/serverManager/service/include/ServiceContext.h +++ b/serverManager/service/include/ServiceContext.h @@ -24,6 +24,7 @@ #include "IServiceContext.h" #include "ISessionServerAppManager.h" #include "IStateObserver.h" +#include #include #include #include diff --git a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp index cf426e201..50237421e 100644 --- a/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp +++ b/tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp @@ -321,8 +321,11 @@ TEST_F(SessionServerAppManagerTests, SessionServerShouldSkipRestart) TEST_F(SessionServerAppManagerTests, SessionServerShouldSkipRestartWhenShuttingDown) { + sessionServerWillLaunch(firebolt::rialto::common::SessionServerState::INACTIVE); + ASSERT_TRUE(triggerInitiateApplication(firebolt::rialto::common::SessionServerState::INACTIVE)); triggerSetShuttingDown(); triggerRestartServer(); + sessionServerWillKillRunningApplication(); } TEST_F(SessionServerAppManagerTests, SessionServerShouldReportStartupTimeout) diff --git a/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp b/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp index e509a452a..0c1c6ad69 100644 --- a/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp +++ b/tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp @@ -90,7 +90,7 @@ TEST_F(ServerManagerServiceTests, registerLogHandlerShouldFailWhenPtrIsNull) EXPECT_FALSE(triggerRegisterLogHandler(nullptr)); } -TEST(ServiceContextTests, DestructorShouldMarkSessionServerAppManagerAsShuttingDown) +TEST(ServiceContextTests, DestructorShouldNotThrow) { ASSERT_NO_THROW(createAndDestroyServiceContext()); }