Fix Docker state drift: add 5s background state poller and make mutating operations idempotent (404/304 as success)

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)
 

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
 

main.cpp

@@ -1,4 +1,5 @@
 #include "src/docker_manager.hpp"
+#include "src/docker_state_poller.hpp"
 #include "src/operation_state.hpp"
 
 #include <cctype>
@@ -153,6 +154,7 @@ private:
     std::atomic<bool> spinner_stop_{true};
     std::vector<std::thread> 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<DockerManager::ContainerInfo> containers,
-                             DockerManager::ListResult<DockerManager::ImageInfo> images);
+                             DockerManager::ListResult<DockerManager::ImageInfo> images,
+                             bool silent = false);
     static void ClearTerminal();
     void RunWithRestoredIO(const std::function<void()>&, bool clear_before = false,
                            bool clear_after = false);
@@ -282,7 +285,8 @@ void TuxDockApp::RefreshState(const std::string& message) {
 
 void TuxDockApp::ApplyRefreshResults(
     DockerManager::ListResult<DockerManager::ContainerInfo> containers,
-    DockerManager::ListResult<DockerManager::ImageInfo> images) {
+    DockerManager::ListResult<DockerManager::ImageInfo> 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<void()> fn) {
+            if (ftxui::ScreenInteractive::Active() == &screen) {
+                screen.Post(std::move(fn));
+            }
+        },
+        [this](DockerManager::ListResult<DockerManager::ContainerInfo> containers,
+               DockerManager::ListResult<DockerManager::ImageInfo> 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();

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_;

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<DockerEngineClient>()) {}
+
+DockerManager::DockerManager(std::unique_ptr<DockerEngineClient> 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<std::string>& args,
 }
 
 bool DockerManager::checkConnection(std::string& error) const {
-    return engine_.checkConnection(error);
+    return engine_->checkConnection(error);
 }
 
 DockerManager::ListResult<DockerManager::ContainerInfo> DockerManager::getContainerList() const {
     ListResult<ContainerInfo> 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::ContainerInfo> DockerManager::getContai
 
 DockerManager::ListResult<DockerManager::ImageInfo> DockerManager::getImageList() const {
     ListResult<ImageInfo> 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 {

src/docker_manager.hpp

@@ -2,12 +2,16 @@
 
 #include "docker_engine_client.hpp"
 
+#include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 class DockerManager {
 public:
+    DockerManager();
+    explicit DockerManager(std::unique_ptr<DockerEngineClient> 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<DockerEngineClient> engine_;
 
     static std::string processError(const std::string& fallback,
                                     const std::string& stderr_text);

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<std::mutex> lock(mutex_);
+        stop_ = false;
+        immediate_ = false;
+    }
+    thread_ = std::thread(&DockerStatePoller::loop, this);
+}
+
+void DockerStatePoller::stop() {
+    {
+        std::lock_guard<std::mutex> lock(mutex_);
+        stop_ = true;
+    }
+    cv_.notify_all();
+    if (thread_.joinable()) thread_.join();
+}
+
+void DockerStatePoller::trigger_now() {
+    {
+        std::lock_guard<std::mutex> lock(mutex_);
+        immediate_ = true;
+    }
+    cv_.notify_all();
+}
+
+void DockerStatePoller::loop() {
+    while (true) {
+        {
+            std::unique_lock<std::mutex> 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));
+        });
+    }
+}

src/docker_state_poller.hpp

@@ -0,0 +1,42 @@
+#pragma once
+
+#include "docker_manager.hpp"
+
+#include <chrono>
+#include <condition_variable>
+#include <functional>
+#include <mutex>
+#include <thread>
+
+class DockerStatePoller {
+public:
+    using ResultCallback = std::function<void(DockerManager::ListResult<DockerManager::ContainerInfo>,
+                                              DockerManager::ListResult<DockerManager::ImageInfo>)>;
+    using PostFunction = std::function<void(std::function<void()>)>;
+
+    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};
+};

tests/test_docker_manager_idempotent.cpp

@@ -0,0 +1,136 @@
+#include "docker_manager.hpp"
+#include "docker_engine_client.hpp"
+
+#include <cassert>
+#include <iostream>
+#include <memory>
+#include <string>
+
+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<MockEngineClient> make_engine(const EngineResponse& response) {
+    auto engine = std::make_unique<MockEngineClient>();
+    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";
+}

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 <cassert>
+#include <chrono>
+#include <condition_variable>
+#include <iostream>
+#include <memory>
+#include <mutex>
+
+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<std::mutex> lock(mutex);
+        return cv.wait_for(lock, timeout, [this, n] { return count >= n; });
+    }
+};
+
+static std::unique_ptr<DockerManager> make_manager(const std::string& containers_json,
+                                                   const std::string& images_json) {
+    auto engine = std::make_unique<FakeEngineClient>();
+    engine->container_response = EngineResponse{200, containers_json, {}};
+    engine->image_response = EngineResponse{200, images_json, {}};
+    return std::make_unique<DockerManager>(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<void()> fn) { fn(); },
+        [&state](auto containers, auto images) {
+            std::lock_guard<std::mutex> 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<void()> fn) { fn(); },
+        [&state](auto containers, auto images) {
+            std::lock_guard<std::mutex> 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<void()>) {}, [](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";
+}