diff --git a/CMakeLists.txt b/CMakeLists.txt index 5181a7c..8256449 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,6 +38,7 @@ add_executable(tux-dock src/operation_state.cpp src/stop_waiter.cpp src/docker_manager.cpp + src/docker_state_poller.cpp ) target_link_libraries(tux-dock @@ -93,6 +94,35 @@ if(BUILD_TESTING) target_compile_options(tux-dock-stop-sequence-tests PRIVATE -Wall -Wextra -Wpedantic) add_test(NAME tux-dock-stop-sequence-tests COMMAND tux-dock-stop-sequence-tests) + add_executable(tux-dock-manager-idempotent-tests + tests/test_docker_manager_idempotent.cpp + src/docker_manager.cpp + src/docker_engine_client.cpp + src/http_response_parser.cpp + src/container_parser.cpp + src/process_runner.cpp + src/stop_waiter.cpp + ) + target_include_directories(tux-dock-manager-idempotent-tests PRIVATE src) + target_link_libraries(tux-dock-manager-idempotent-tests PRIVATE nlohmann_json::nlohmann_json) + target_compile_options(tux-dock-manager-idempotent-tests PRIVATE -Wall -Wextra -Wpedantic) + add_test(NAME tux-dock-manager-idempotent-tests COMMAND tux-dock-manager-idempotent-tests) + + add_executable(tux-dock-state-poller-tests + tests/test_docker_state_poller.cpp + src/docker_state_poller.cpp + src/docker_manager.cpp + src/docker_engine_client.cpp + src/http_response_parser.cpp + src/container_parser.cpp + src/process_runner.cpp + src/stop_waiter.cpp + ) + target_include_directories(tux-dock-state-poller-tests PRIVATE src) + target_link_libraries(tux-dock-state-poller-tests PRIVATE nlohmann_json::nlohmann_json) + target_compile_options(tux-dock-state-poller-tests PRIVATE -Wall -Wextra -Wpedantic) + add_test(NAME tux-dock-state-poller-tests COMMAND tux-dock-state-poller-tests) + add_test(NAME tux-dock-docker-integration-tests COMMAND sh ${CMAKE_CURRENT_SOURCE_DIR}/tests/test_docker_integration.sh) diff --git a/DEVLOG.md b/DEVLOG.md index 58906d5..933b330 100644 --- a/DEVLOG.md +++ b/DEVLOG.md @@ -6,6 +6,10 @@ - Fixed duplicate port mappings shown in the container list TUI. Docker's `/containers/json` API returns separate `Ports` entries for IPv4 (`0.0.0.0`) and IPv6 (`::`) bindings of the same host port; the container parser now deduplicates these into a single `public:private` display string. - Added a container parser regression test covering IPv4/IPv6 duplicate port bindings. +- Added `DockerStatePoller`, a background thread that silently refreshes cached container/image state from Docker every 5 seconds. This prevents state drift when an Engine API call times out but Docker still applied the change. +- Made mutating Engine API operations idempotent: `removeContainer` treats `404` as success, `deleteImage` treats `404` as success, and `startDetached` treats `304` as success, eliminating spurious "already removed/started" failures on retry. +- Refactored `DockerManager` to accept an injectable `DockerEngineClient` for unit testing. +- Added unit tests for idempotent Docker manager operations and for `DockerStatePoller` lifecycle, periodic polling, and immediate refresh triggers. ### Version diff --git a/main.cpp b/main.cpp index 257b4aa..e19ef65 100644 --- a/main.cpp +++ b/main.cpp @@ -1,4 +1,5 @@ #include "src/docker_manager.hpp" +#include "src/docker_state_poller.hpp" #include "src/operation_state.hpp" #include @@ -153,6 +154,7 @@ private: std::atomic spinner_stop_{true}; std::vector action_threads_; OperationState operation_state_; + DockerStatePoller state_poller_; std::size_t spinner_frame_ = 0; static constexpr int kMaxStopTimeout = 300; @@ -201,7 +203,8 @@ private: void BeginStopOperation(const std::string& id, int timeout_seconds = 10); void RefreshState(const std::string& message = "Refreshing Docker state..."); void ApplyRefreshResults(DockerManager::ListResult containers, - DockerManager::ListResult images); + DockerManager::ListResult images, + bool silent = false); static void ClearTerminal(); void RunWithRestoredIO(const std::function&, bool clear_before = false, bool clear_after = false); @@ -282,7 +285,8 @@ void TuxDockApp::RefreshState(const std::string& message) { void TuxDockApp::ApplyRefreshResults( DockerManager::ListResult containers, - DockerManager::ListResult images) { + DockerManager::ListResult images, + bool silent) { std::string error; if (containers.ok()) { containers_ = std::move(containers.items); @@ -295,7 +299,9 @@ void TuxDockApp::ApplyRefreshResults( if (!error.empty()) error += "\n"; error += "Images: " + images.error; } - SetStatus(error.empty() ? "Docker state refreshed." : "Refresh failed; cached state preserved.\n" + error); + if (!silent) { + SetStatus(error.empty() ? "Docker state refreshed." : "Refresh failed; cached state preserved.\n" + error); + } } void TuxDockApp::OpenInput(const std::string& title, @@ -474,7 +480,7 @@ void TuxDockApp::BeginBusyOperation(const std::string& title, StopSpinner(); modal_mode_ = ModalMode::None; OpenMessage("Operation complete", message); - RefreshState(); + state_poller_.trigger_now(); }); active->PostEvent(ftxui::Event::Custom); } @@ -495,7 +501,7 @@ void TuxDockApp::BeginStopOperation(const std::string& id, int timeout_seconds) if (!active || ftxui::ScreenInteractive::Active() != active) return; active->Post([this, stopped, message, containers = std::move(containers), images = std::move(images)]() mutable { RemoveSignalTraps(); - ApplyRefreshResults(std::move(containers), std::move(images)); + ApplyRefreshResults(std::move(containers), std::move(images), /*silent=*/true); operation_state_.complete(message); StopSpinner(); modal_mode_ = ModalMode::None; @@ -884,7 +890,19 @@ int TuxDockApp::Run() { }); auto screen = ftxui::ScreenInteractive::TerminalOutput(); screen_ = &screen; + state_poller_.start( + docker_, + [&screen](std::function fn) { + if (ftxui::ScreenInteractive::Active() == &screen) { + screen.Post(std::move(fn)); + } + }, + [this](DockerManager::ListResult containers, + DockerManager::ListResult images) { + ApplyRefreshResults(std::move(containers), std::move(images), /*silent=*/true); + }); screen.Loop(app); + state_poller_.stop(); screen_ = nullptr; if (refresh_thread_.joinable()) refresh_thread_.join(); if (spinner_thread_.joinable()) spinner_thread_.join(); diff --git a/src/docker_engine_client.hpp b/src/docker_engine_client.hpp index eb104c8..14eb584 100644 --- a/src/docker_engine_client.hpp +++ b/src/docker_engine_client.hpp @@ -13,13 +13,14 @@ struct EngineResponse { class DockerEngineClient { public: explicit DockerEngineClient(std::string socket_path = "/var/run/docker.sock"); + virtual ~DockerEngineClient() = default; bool checkConnection(std::string& error) const; - EngineResponse request(const std::string& method, - const std::string& path, - const std::string& body = {}, - int timeout_seconds = 10) const; + virtual EngineResponse request(const std::string& method, + const std::string& path, + const std::string& body = {}, + int timeout_seconds = 10) const; private: std::string socket_path_; diff --git a/src/docker_manager.cpp b/src/docker_manager.cpp index 1c2334f..1d14efb 100644 --- a/src/docker_manager.cpp +++ b/src/docker_manager.cpp @@ -22,6 +22,12 @@ std::string apiError(const EngineResponse& response, const std::string& fallback } // namespace +DockerManager::DockerManager() + : engine_(std::make_unique()) {} + +DockerManager::DockerManager(std::unique_ptr engine) + : engine_(std::move(engine)) {} + std::string DockerManager::processError(const std::string& fallback, const std::string& stderr_text) { return stderr_text.empty() ? fallback : stderr_text; @@ -43,12 +49,12 @@ bool DockerManager::runProcess(const std::vector& args, } bool DockerManager::checkConnection(std::string& error) const { - return engine_.checkConnection(error); + return engine_->checkConnection(error); } DockerManager::ListResult DockerManager::getContainerList() const { ListResult result; - const EngineResponse response = engine_.request("GET", "/containers/json?all=true"); + const EngineResponse response = engine_->request("GET", "/containers/json?all=true"); if (!response.ok()) { result.error = apiError(response, "Could not list containers."); return result; @@ -59,7 +65,7 @@ DockerManager::ListResult DockerManager::getContai DockerManager::ListResult DockerManager::getImageList() const { ListResult result; - const EngineResponse response = engine_.request("GET", "/images/json"); + const EngineResponse response = engine_->request("GET", "/images/json"); if (!response.ok()) { result.error = apiError(response, "Could not list images."); return result; @@ -143,23 +149,31 @@ bool DockerManager::createContainer(const std::string& name, } bool DockerManager::startDetached(const std::string& id, std::string& message) const { - const EngineResponse response = engine_.request("POST", "/containers/" + id + "/start"); - if (!response.ok()) { - message = apiError(response, "Could not start that container."); - return false; + const EngineResponse response = engine_->request("POST", "/containers/" + id + "/start"); + if (response.ok()) { + message = "Container started in detached mode."; + return true; } - message = "Container started in detached mode."; - return true; + if (response.status_code == 304) { + message = "Container already started."; + return true; + } + message = apiError(response, "Could not start that container."); + return false; } bool DockerManager::deleteImage(const std::string& id, std::string& message) const { - const EngineResponse response = engine_.request("DELETE", "/images/" + id); - if (!response.ok()) { - message = apiError(response, "Could not delete that image."); - return false; + const EngineResponse response = engine_->request("DELETE", "/images/" + id); + if (response.ok()) { + message = "Image deleted."; + return true; } - message = "Image deleted."; - return true; + if (response.status_code == 404) { + message = "Image already deleted."; + return true; + } + message = apiError(response, "Could not delete that image."); + return false; } bool DockerManager::stopContainer(const std::string& id, @@ -177,7 +191,7 @@ bool DockerManager::stopContainer(const std::string& id, // Docker holds the /stop response until the container stops or the grace // period expires. Use a per-request HTTP timeout longer than the grace // period so the client does not race Docker's own timeout. - const EngineResponse response = engine_.request( + const EngineResponse response = engine_->request( "POST", "/containers/" + id + "/stop?t=" + std::to_string(timeout_seconds), "", @@ -193,13 +207,17 @@ bool DockerManager::stopContainer(const std::string& id, } bool DockerManager::removeContainer(const std::string& id, std::string& message) const { - const EngineResponse response = engine_.request("DELETE", "/containers/" + id); - if (!response.ok()) { - message = apiError(response, "Could not remove that container."); - return false; + const EngineResponse response = engine_->request("DELETE", "/containers/" + id); + if (response.ok()) { + message = "Container removed."; + return true; } - message = "Container removed."; - return true; + if (response.status_code == 404) { + message = "Container already removed."; + return true; + } + message = apiError(response, "Could not remove that container."); + return false; } bool DockerManager::execShell(const std::string& id, std::string& message) const { diff --git a/src/docker_manager.hpp b/src/docker_manager.hpp index 0b4d8ba..492f916 100644 --- a/src/docker_manager.hpp +++ b/src/docker_manager.hpp @@ -2,12 +2,16 @@ #include "docker_engine_client.hpp" +#include #include #include #include class DockerManager { public: + DockerManager(); + explicit DockerManager(std::unique_ptr engine); + struct ContainerInfo { std::string id; std::string name; @@ -47,7 +51,7 @@ public: const std::string& command, std::string& message) const; private: - DockerEngineClient engine_; + std::unique_ptr engine_; static std::string processError(const std::string& fallback, const std::string& stderr_text); diff --git a/src/docker_state_poller.cpp b/src/docker_state_poller.cpp new file mode 100644 index 0000000..0a614db --- /dev/null +++ b/src/docker_state_poller.cpp @@ -0,0 +1,59 @@ +#include "docker_state_poller.hpp" + +DockerStatePoller::~DockerStatePoller() { + stop(); +} + +void DockerStatePoller::start(const DockerManager& docker, + PostFunction post, + ResultCallback callback) { + stop(); + docker_ = &docker; + post_ = std::move(post); + callback_ = std::move(callback); + { + std::lock_guard lock(mutex_); + stop_ = false; + immediate_ = false; + } + thread_ = std::thread(&DockerStatePoller::loop, this); +} + +void DockerStatePoller::stop() { + { + std::lock_guard lock(mutex_); + stop_ = true; + } + cv_.notify_all(); + if (thread_.joinable()) thread_.join(); +} + +void DockerStatePoller::trigger_now() { + { + std::lock_guard lock(mutex_); + immediate_ = true; + } + cv_.notify_all(); +} + +void DockerStatePoller::loop() { + while (true) { + { + std::unique_lock lock(mutex_); + cv_.wait_for(lock, kInterval, [this] { return stop_ || immediate_; }); + if (stop_) break; + immediate_ = false; + } + + if (!docker_ || !post_ || !callback_) continue; + + auto containers = docker_->getContainerList(); + auto images = docker_->getImageList(); + + post_([callback = callback_, + containers = std::move(containers), + images = std::move(images)]() mutable { + callback(std::move(containers), std::move(images)); + }); + } +} diff --git a/src/docker_state_poller.hpp b/src/docker_state_poller.hpp new file mode 100644 index 0000000..8ce663d --- /dev/null +++ b/src/docker_state_poller.hpp @@ -0,0 +1,42 @@ +#pragma once + +#include "docker_manager.hpp" + +#include +#include +#include +#include +#include + +class DockerStatePoller { +public: + using ResultCallback = std::function, + DockerManager::ListResult)>; + using PostFunction = std::function)>; + + DockerStatePoller() = default; + ~DockerStatePoller(); + + DockerStatePoller(const DockerStatePoller&) = delete; + DockerStatePoller& operator=(const DockerStatePoller&) = delete; + + void start(const DockerManager& docker, + PostFunction post, + ResultCallback callback); + void stop(); + void trigger_now(); + +private: + void loop(); + + const DockerManager* docker_ = nullptr; + PostFunction post_; + ResultCallback callback_; + std::thread thread_; + std::mutex mutex_; + std::condition_variable cv_; + bool stop_ = false; + bool immediate_ = false; + + static constexpr std::chrono::seconds kInterval{5}; +}; diff --git a/tests/test_docker_manager_idempotent.cpp b/tests/test_docker_manager_idempotent.cpp new file mode 100644 index 0000000..23acc47 --- /dev/null +++ b/tests/test_docker_manager_idempotent.cpp @@ -0,0 +1,136 @@ +#include "docker_manager.hpp" +#include "docker_engine_client.hpp" + +#include +#include +#include +#include + +class MockEngineClient : public DockerEngineClient { +public: + mutable std::string last_method; + mutable std::string last_path; + EngineResponse response; + + EngineResponse request(const std::string& method, + const std::string& path, + const std::string& /*body*/ = {}, + int /*timeout_seconds*/ = 10) const override { + last_method = method; + last_path = path; + return response; + } +}; + +static std::unique_ptr make_engine(const EngineResponse& response) { + auto engine = std::make_unique(); + engine->response = response; + return engine; +} + +void test_start_detached() { + { + auto engine = make_engine(EngineResponse{204, {}, {}}); + auto* raw = engine.get(); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.startDetached("abc123", message)); + assert(message == "Container started in detached mode."); + assert(raw->last_method == "POST"); + assert(raw->last_path == "/containers/abc123/start"); + } + { + auto engine = make_engine(EngineResponse{304, {}, {}}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.startDetached("abc123", message)); + assert(message == "Container already started."); + } + { + auto engine = make_engine(EngineResponse{500, {}, "engine error"}); + DockerManager manager(std::move(engine)); + std::string message; + assert(!manager.startDetached("abc123", message)); + assert(message == "engine error"); + } +} + +void test_remove_container() { + { + auto engine = make_engine(EngineResponse{204, {}, {}}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.removeContainer("abc123", message)); + assert(message == "Container removed."); + } + { + auto engine = make_engine(EngineResponse{404, {}, "No such container"}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.removeContainer("abc123", message)); + assert(message == "Container already removed."); + } + { + auto engine = make_engine(EngineResponse{409, {}, "conflict"}); + DockerManager manager(std::move(engine)); + std::string message; + assert(!manager.removeContainer("abc123", message)); + assert(message == "conflict"); + } +} + +void test_delete_image() { + { + auto engine = make_engine(EngineResponse{200, {}, {}}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.deleteImage("sha256:abc", message)); + assert(message == "Image deleted."); + } + { + auto engine = make_engine(EngineResponse{404, {}, "No such image"}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.deleteImage("sha256:abc", message)); + assert(message == "Image already deleted."); + } + { + auto engine = make_engine(EngineResponse{409, {}, "image in use"}); + DockerManager manager(std::move(engine)); + std::string message; + assert(!manager.deleteImage("sha256:abc", message)); + assert(message == "image in use"); + } +} + +void test_stop_container() { + { + auto engine = make_engine(EngineResponse{204, {}, {}}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.stopContainer("abc123", message, 10)); + assert(message == "Container stopped."); + } + { + auto engine = make_engine(EngineResponse{304, {}, {}}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.stopContainer("abc123", message, 10)); + assert(message == "Container stopped."); + } + { + auto engine = make_engine(EngineResponse{404, {}, "No such container"}); + DockerManager manager(std::move(engine)); + std::string message; + assert(manager.stopContainer("abc123", message, 10)); + assert(message == "Container stopped."); + } +} + +int main() { + test_start_detached(); + test_remove_container(); + test_delete_image(); + test_stop_container(); + std::cout << "Docker manager idempotent operation tests passed\n"; +} diff --git a/tests/test_docker_state_poller.cpp b/tests/test_docker_state_poller.cpp new file mode 100644 index 0000000..e914638 --- /dev/null +++ b/tests/test_docker_state_poller.cpp @@ -0,0 +1,121 @@ +#include "docker_state_poller.hpp" +#include "docker_manager.hpp" +#include "docker_engine_client.hpp" + +#include +#include +#include +#include +#include +#include + +class FakeEngineClient : public DockerEngineClient { +public: + EngineResponse container_response; + EngineResponse image_response; + + EngineResponse request(const std::string& method, + const std::string& path, + const std::string& /*body*/ = {}, + int /*timeout_seconds*/ = 10) const override { + (void)method; + if (path == "/containers/json?all=true") return container_response; + if (path == "/images/json") return image_response; + return EngineResponse{}; + } +}; + +struct CallbackState { + std::mutex mutex; + std::condition_variable cv; + int count = 0; + std::size_t container_count = 0; + std::size_t image_count = 0; + bool last_containers_ok = false; + bool last_images_ok = false; + + bool wait_for_count(int n, std::chrono::milliseconds timeout) { + std::unique_lock lock(mutex); + return cv.wait_for(lock, timeout, [this, n] { return count >= n; }); + } +}; + +static std::unique_ptr make_manager(const std::string& containers_json, + const std::string& images_json) { + auto engine = std::make_unique(); + engine->container_response = EngineResponse{200, containers_json, {}}; + engine->image_response = EngineResponse{200, images_json, {}}; + return std::make_unique(std::move(engine)); +} + +void test_trigger_now() { + auto manager = make_manager( + R"([{"Id":"c1","Names":["/web"],"State":"running","Ports":[]}])", + "[]"); + + CallbackState state; + DockerStatePoller poller; + poller.start( + *manager, + [](std::function fn) { fn(); }, + [&state](auto containers, auto images) { + std::lock_guard lock(state.mutex); + ++state.count; + state.container_count = containers.items.size(); + state.image_count = images.items.size(); + state.last_containers_ok = containers.ok(); + state.last_images_ok = images.ok(); + state.cv.notify_all(); + }); + + poller.trigger_now(); + assert(state.wait_for_count(1, std::chrono::seconds(2))); + assert(state.container_count == 1); + assert(state.image_count == 0); + assert(state.last_containers_ok); + assert(state.last_images_ok); + + poller.stop(); +} + +void test_periodic_polling() { + auto manager = make_manager( + R"([{"Id":"c1","Names":["/web"],"State":"running","Ports":[]}])", + "[]"); + + CallbackState state; + DockerStatePoller poller; + poller.start( + *manager, + [](std::function fn) { fn(); }, + [&state](auto containers, auto images) { + std::lock_guard lock(state.mutex); + ++state.count; + state.container_count = containers.items.size(); + state.image_count = images.items.size(); + state.cv.notify_all(); + }); + + assert(state.wait_for_count(2, std::chrono::seconds(12))); + assert(state.container_count == 1); + assert(state.image_count == 0); + + poller.stop(); +} + +void test_stop_does_not_hang() { + auto manager = make_manager("[]", "[]"); + DockerStatePoller poller; + poller.start(*manager, [](std::function) {}, [](auto, auto) {}); + auto start = std::chrono::steady_clock::now(); + poller.stop(); + auto elapsed = std::chrono::steady_clock::now() - start; + assert(elapsed < std::chrono::seconds(1)); +} + +int main() { + test_trigger_now(); + test_periodic_polling(); + test_stop_does_not_hang(); + std::cout << "Docker state poller tests passed\n"; +}