diff --git a/daemon/lib/source/DobbyManager.cpp b/daemon/lib/source/DobbyManager.cpp index 6c6ed2ae..3b40bba6 100644 --- a/daemon/lib/source/DobbyManager.cpp +++ b/daemon/lib/source/DobbyManager.cpp @@ -1310,6 +1310,48 @@ bool DobbyManager::restartContainer(const ContainerId &id, return true; } +// ----------------------------------------------------------------------------- +/** + * @brief Sends SIGKILL to every process in the container's cgroup and + * confirms it actually died. + * + * A crashed or hung process inside the container can leave orphaned + * descendants that the container's init hasn't (or, if itself stuck, can't) + * reaped, which would otherwise leave the container running forever from + * runc/Dobby's point of view. Killing the whole cgroup (rather than just the + * tracked init process) means the container is torn down even if init isn't + * able to clean up after its children itself. + * + * SIGKILL can't be masked or ignored, but a process stuck in an + * uninterruptible sleep (D state) won't die until it exits that syscall, so + * this waits for the container to actually leave the Running state instead + * of just trusting that sending the signal was enough. + * + * @param[in] id The id of the container to kill. + * + * @return true if the container was confirmed stopped, false if it is still + * running after all retries (almost always a process wedged in an + * uninterruptible sleep, which no amount of signalling can fix). + */ +bool DobbyManager::forceKillContainerAndVerify(const ContainerId &id) +{ + mRunc->killCont(id, SIGKILL, /*all=*/true); + + const int maxRetry = 10; + for (int attempt = 1; attempt <= maxRetry; attempt++) + { + /* coverity[sleep : FALSE] */ + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + + if (mRunc->state(id) != DobbyRunC::ContainerStatus::Running) + return true; + } + + AI_LOG_ERROR("SIGKILL did not stop container '%s' - a process is likely " + "stuck in an uninterruptible sleep", id.c_str()); + return false; +} + // ----------------------------------------------------------------------------- /** * @brief Stops a running container @@ -1317,15 +1359,19 @@ bool DobbyManager::restartContainer(const ContainerId &id, * If withPrejudice is not specified (the default) then we send the init * process within the container a SIGTERM. * - * If the withPrejudice is true then we use the SIGKILL signal. + * If the withPrejudice is true then we SIGKILL every process in the + * container's cgroup (not just the tracked init process) and block for up + * to ~500ms confirming the container actually stopped, so a hung/crashed + * process with orphaned descendants can't leave the container running + * forever. * - * The kill signal itself is sent asynchronously (the actual container teardown - * happens in the background and @a mContainerStoppedCb is called when it - * completes). However, if the container is in the Hibernating state when this - * is called, the function will block for up to DobbyHibernate::DFL_TIMEOUTE_MS - * while aborting the in-progress hibernation via WakeupProcess() before - * sending the kill signal. This is necessary to ensure memcr_worker has - * unseized the in-flight PID before it is killed. + * A plain (non-prejudice) SIGTERM is sent asynchronously (the actual + * container teardown happens in the background and @a mContainerStoppedCb + * is called when it completes). However, if the container is in the + * Hibernating state when this is called, the function will block for up to + * DobbyHibernate::DFL_TIMEOUTE_MS while aborting the in-progress hibernation + * via WakeupProcess() before sending the kill signal. This is necessary to + * ensure memcr_worker has unseized the in-flight PID before it is killed. * * The @a mContainerStoppedCb callback will be called when the container * has actually been torn down. @@ -1334,8 +1380,9 @@ bool DobbyManager::restartContainer(const ContainerId &id, * @param[in] withPrejudice If true the container process is killed with * SIGKILL, otherwise SIGTERM is used. * - * @return true if a container with a matching id was found and a signal - * sent successfully to it. + * @return true if a container with a matching id was found and, for a + * SIGTERM, the signal was sent successfully, or for a SIGKILL, the + * container was confirmed stopped. */ bool DobbyManager::stopContainer(int32_t cd, bool withPrejudice) { @@ -1403,7 +1450,15 @@ bool DobbyManager::stopContainer(int32_t cd, bool withPrejudice) } } - if (!mRunc->killCont(id, withPrejudice ? SIGKILL : SIGTERM)) + if (withPrejudice) + { + if (!forceKillContainerAndVerify(id)) + { + AI_LOG_FN_EXIT(); + return false; + } + } + else if (!mRunc->killCont(id, SIGTERM)) { AI_LOG_WARN("failed to send signal to '%s'", id.c_str()); AI_LOG_FN_EXIT(); @@ -1436,10 +1491,8 @@ bool DobbyManager::stopContainer(int32_t cd, bool withPrejudice) } // Container has been resumed, so kill it now - if (!mRunc->killCont(id, SIGKILL)) - + if (!forceKillContainerAndVerify(id)) { - AI_LOG_WARN("failed to send signal to '%s'", id.c_str()); AI_LOG_FN_EXIT(); return false; } diff --git a/daemon/lib/source/include/DobbyManager.h b/daemon/lib/source/include/DobbyManager.h index aa688890..605c1908 100644 --- a/daemon/lib/source/include/DobbyManager.h +++ b/daemon/lib/source/include/DobbyManager.h @@ -184,6 +184,8 @@ class DobbyManager bool abortContainerHibernationIfNeeded(int32_t cd); + bool forceKillContainerAndVerify(const ContainerId& id); + private: ContainerStartedFunc mContainerStartedCb; ContainerStoppedFunc mContainerStoppedCb; diff --git a/openspec/specs/daemon-core.md b/openspec/specs/daemon-core.md index 0042c10e..bd9139fb 100644 --- a/openspec/specs/daemon-core.md +++ b/openspec/specs/daemon-core.md @@ -21,6 +21,7 @@ The daemon is the heart of Dobby, orchestrating container creation, start, stop, - Invokes legacy plugin hooks (PostConstruction, PreStart, PostStart, PostStop, PreDestruction) and RDK plugin hooks (postInstallation, preCreation, postHalt) - Supports `restartOnCrash` for automatic container restart - Loads plugins from configurable `PLUGIN_PATH` (default: `/usr/lib/plugins/dobby`) +- `stopContainer(cd, withPrejudice=true)` sends SIGKILL to every process in the container's cgroup (`killCont(..., all=true)`), not just the tracked init process, and blocks for up to ~500ms confirming via `DobbyRunC::state()` that the container actually stopped. This guarantees termination even if a crashed/hung process left orphaned descendants that init can't reap; it only fails if a process is wedged in an uninterruptible (D-state) sleep. ### DobbyContainer - Stores container state: bundle, config, rootfs, rdkPluginManager @@ -161,3 +162,4 @@ _No open queries._ ## Change History - 2025-05-18 - openspec-templater - Restructured to match spec template. +- 2026-09-16 - Hardened `stopContainer(withPrejudice=true)` to SIGKILL the whole container cgroup and verify the container actually stopped, so a hung/crashed process with orphaned descendants can't leave the container running forever. diff --git a/tests/L1_testing/tests/DobbyManagerTest/DaemonDobbyManagerTest.cpp b/tests/L1_testing/tests/DobbyManagerTest/DaemonDobbyManagerTest.cpp index 909695ea..270e9d63 100755 --- a/tests/L1_testing/tests/DobbyManagerTest/DaemonDobbyManagerTest.cpp +++ b/tests/L1_testing/tests/DobbyManagerTest/DaemonDobbyManagerTest.cpp @@ -3115,6 +3115,73 @@ TEST_F(DaemonDobbyManagerTest, stopContainer_FailedToSendSignal) expect_cleanupContainersShutdown(); } +/** + * @brief Test stopContainer with withPrejudice=true. + * Check that the force-kill path sends SIGKILL to the whole cgroup + * (all=true), not just the tracked init process, and that stopContainer + * returns true once runc reports the container has actually stopped. + * + * @return true. + */ +TEST_F(DaemonDobbyManagerTest, stopContainer_ForceKill_SendsSigkillToWholeCgroup) +{ + int32_t cd = 1234; + ContainerId id = ContainerId::create("container1"); + expect_invalidContainerCleanupTask(); + + expect_startContainerFromBundle(cd,id); + + bool killedWithAllFlag = false; + EXPECT_CALL(*p_runcMock, killCont(::testing::_, SIGKILL, ::testing::_)) + .Times(1) + .WillOnce(::testing::Invoke( + [&killedWithAllFlag](const ContainerId &id, int signal, bool all) { + killedWithAllFlag = all; + return true; + })); + + EXPECT_CALL(*p_runcMock, state(::testing::_)) + .Times(1) + .WillOnce(::testing::Return(DobbyRunC::ContainerStatus::Stopped)); + + int return_value = dobbyManager_test->stopContainer(cd, true); + EXPECT_EQ(return_value, true); + EXPECT_TRUE(killedWithAllFlag); + + expect_cleanupContainersShutdown(); +} + +/** + * @brief Test stopContainer with withPrejudice=true. + * Check that if runc still reports the container as Running after SIGKILL + * has been sent (e.g. a process is stuck in an uninterruptible sleep), + * stopContainer gives up after retrying and returns false rather than + * claiming success. + * + * @return false. + */ +TEST_F(DaemonDobbyManagerTest, stopContainer_ForceKill_ContainerStaysRunning_ReturnsFalse) +{ + int32_t cd = 1234; + ContainerId id = ContainerId::create("container1"); + expect_invalidContainerCleanupTask(); + + expect_startContainerFromBundle(cd,id); + + EXPECT_CALL(*p_runcMock, killCont(::testing::_, SIGKILL, ::testing::_)) + .Times(1) + .WillOnce(::testing::Return(true)); + + EXPECT_CALL(*p_runcMock, state(::testing::_)) + .Times(::testing::AtLeast(1)) + .WillRepeatedly(::testing::Return(DobbyRunC::ContainerStatus::Running)); + + int return_value = dobbyManager_test->stopContainer(cd, true); + EXPECT_EQ(return_value, false); + + expect_cleanupContainersShutdown(); +} + /* ----------------------------------------------------------------------------- * @brief Gets the stats for the container *