Repository: eclipse-bluechi/bluechi (language: python)

Issue:
valgrind error in integration test: Leak in manager_add_node (manager.c:256)
## Describe the bug

Integration test `fail /tests/tier0/monitor-agent-loses-connection` fails when run with valgrind, reporting the following error:

```
==49== Memcheck, a memory error detector
==49== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==49== Using Valgrind-3.21.0 and LibVEX; rerun with -h for copyright info
==49== Command: /usr/libexec/bluechi-agent
==49== Parent PID: 1
==49== 
==49== 
==49== HEAP SUMMARY:
==49==     in use at exit: 0 bytes in 0 blocks
==49==   total heap usage: 1,932 allocs, 1,932 frees, 307,834 bytes allocated
==49== 
==49== All heap blocks were freed -- no leaks are possible
==49== 
==49== For lists of detected and suppressed errors, rerun with: -s
==49== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
InlinedFun (node.c:132)
==44==    by 0x1133D6: manager_add_node (manager.c:256)
==44==    by 0x10D8ED: UnknownInlinedFun (manager.c:339)
==44==    by 0x10D8ED: main (main.c:88)
==44== 
==44== LEAK SUMMARY:
==44==    definitely lost: 128 bytes in 1 blocks
==44==    indirectly lost: 17,436 bytes in 191 blocks
==44==      possibly lost: 0 bytes in 0 blocks
==44==    still reachable: 0 bytes in 0 blocks
==44==         suppressed: 0 bytes in 0 blocks
==44== 
==44== For lists of detected and suppressed errors, rerun with: -s
==44== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
```

## To Reproduce

Please provide the steps to reproduce the issue.

1. Run integration tests with valgrind (after #575  merged)
2. See valgrind reports in the collected logs



Discussion / hints:
It happens when `bluechi-controller.service` is stopped via `bluechi-agent` itself running on the same machine. 
Reproduces 100% of the times in integration tests, but only sometimes on my laptop

Fix patch:
diff --git a/src/manager/manager.c b/src/manager/manager.c
index 57286c9c7c..cc7244d633 100644
--- a/src/manager/manager.c
+++ b/src/manager/manager.c
@@ -83,9 +83,11 @@ void manager_unref(Manager *manager) {
         Node *node = NULL;
         Node *next_node = NULL;
         LIST_FOREACH_SAFE(nodes, node, next_node, manager->nodes) {
+                node_shutdown(node);
                 node_unref(node);
         }
         LIST_FOREACH_SAFE(nodes, node, next_node, manager->anonymous_nodes) {
+                node_shutdown(node);
                 node_unref(node);
         }
 
diff --git a/src/manager/node.c b/src/manager/node.c
index 839b2e87eb..d2b4cb8036 100644
--- a/src/manager/node.c
+++ b/src/manager/node.c
@@ -169,6 +169,8 @@ Node *node_new(Manager *manager, const char *name) {
                 }
         }
 
+        node->is_shutdown = false;
+
         return steal_pointer(&node);
 }
 
@@ -206,6 +208,15 @@ void node_unref(Node *node) {
         free(node);
 }
 
+void node_shutdown(Node *node) {
+        AgentRequest *req = NULL;
+        AgentRequest *next_req = NULL;
+        node->is_shutdown = true;
+        LIST_FOREACH_SAFE(outstanding_requests, req, next_req, node->outstanding_requests) {
+                agent_request_cancel(req);
+        }
+}
+
 bool node_export(Node *node) {
         Manager *manager = node->manager;
 
@@ -1070,6 +1081,7 @@ int node_create_request(
         req->cb = cb;
         req->userdata = userdata;
         req->free_userdata = free_userdata;
+        req->is_cancelled = false;
         LIST_APPEND(outstanding_requests, node->outstanding_requests, req);
 
         *ret = req;
@@ -1078,10 +1090,24 @@ int node_create_request(
 
 static int agent_request_callback(sd_bus_message *m, void *userdata, UNUSED sd_bus_error *ret_error) {
         _cleanup_agent_request_ AgentRequest *req = userdata;
+        if (req->is_cancelled) {
+                bc_log_debugf("Response received to a cancelled request for node %s. Dropping message.",
+                              req->node->name);
+                return 0;
+        }
 
         return req->cb(req, m, ret_error);
 }
 
+int agent_request_cancel(AgentRequest *r) {
+        _cleanup_agent_request_ AgentRequest *req = r;
+        req->is_cancelled = true;
+        _cleanup_sd_bus_message_ sd_bus_message *m = NULL;
+        sd_bus_message_new_method_errorf(req->message, &m, SD_BUS_ERROR_FAILED, "Request cancelled");
+
+        return req->cb(req, m, NULL);
+}
+
 int agent_request_start(AgentRequest *req) {
         Node *node = req->node;
 
@@ -1156,6 +1182,11 @@ static int method_list_units_callback(AgentRequest *req, sd_bus_message *m, UNUS
 static int node_method_list_units(sd_bus_message *m, void *userdata, UNUSED sd_bus_error *ret_error) {
         Node *node = userdata;
 
+        if (node->is_shutdown) {
+                return sd_bus_reply_method_errorf(
+                                m, SD_BUS_ERROR_FAILED, "Request not allowed: node is in shutdown state");
+        }
+
         _cleanup_agent_request_ AgentRequest *agent_req = node_request_list_units(
                         node,
                         method_list_units_callback,
@@ -1199,6 +1230,11 @@ static int node_method_set_unit_properties(sd_bus_message *m, void *userdata, UN
         const char *unit = NULL;
         int runtime = 0;
 
+        if (node->is_shutdown) {
+                return sd_bus_reply_method_errorf(
+                                m, SD_BUS_ERROR_FAILED, "Request not allowed: node is in shutdown state");
+        }
+
         int r = sd_bus_message_read(m, "sb", &unit, &runtime);
         if (r < 0) {
                 return sd_bus_reply_method_errorf(
@@ -1282,6 +1318,12 @@ static int node_method_passthrough_to_agent_callback(
 
 static int node_method_passthrough_to_agent(sd_bus_message *m, void *userdata, UNUSED sd_bus_error *ret_error) {
         Node *node = userdata;
+
+        if (node->is_shutdown) {
+                return sd_bus_reply_method_errorf(
+                                m, SD_BUS_ERROR_FAILED, "Request not allowed: node is in shutdown state");
+        }
+
         _cleanup_agent_request_ AgentRequest *req = NULL;
         int r = node_create_request(
                         &req,
@@ -1382,6 +1424,11 @@ static int node_run_unit_lifecycle_method(
         const char *mode = NULL;
         uint64_t start_time = get_time_micros();
 
+        if (node->is_shutdown) {
+                return sd_bus_reply_method_errorf(
+                                m, SD_BUS_ERROR_FAILED, "Request not allowed: node is in shutdown state");
+        }
+
         int r = sd_bus_message_read(m, "ss", &unit, &mode);
         if (r < 0) {
                 return sd_bus_reply_method_errorf(
diff --git a/src/manager/node.h b/src/manager/node.h
index 950c8defdc..086bd714eb 100644
--- a/src/manager/node.h
+++ b/src/manager/node.h
@@ -21,6 +21,8 @@ struct AgentRequest {
         free_func_t free_userdata;
         agent_request_response_t cb;
 
+        bool is_cancelled;
+
         LIST_FIELDS(AgentRequest, outstanding_requests);
 };
 
@@ -29,6 +31,8 @@ void agent_request_unref(AgentRequest *req);
 
 int agent_request_start(AgentRequest *req);
 
+int agent_request_cancel(AgentRequest *r);
+
 struct Node {
         int ref_count;
 
@@ -54,11 +58,14 @@ struct Node {
 
         struct hashmap *unit_subscriptions;
         time_t last_seen;
+
+        bool is_shutdown;
 };
 
 Node *node_new(Manager *manager, const char *name);
 Node *node_ref(Node *node);
 void node_unref(Node *node);
+void node_shutdown(Node *node);
 
 const char *node_get_status(Node *node);
 


Test patch:
diff --git a/tests/bluechi_test/container.py b/tests/bluechi_test/container.py
index af6fdfabcb..8d53d92311 100644
--- a/tests/bluechi_test/container.py
+++ b/tests/bluechi_test/container.py
@@ -148,11 +148,11 @@ def wait_for_unit_state_to_be(
         return False
 
     def enable_valgrind(self) -> None:
-        self.exec_run(f"sed -i '/ExecStart=/c\\ExecStart=/usr/bin/valgrind --leak-check=yes "
+        self.exec_run(f"sed -i '/ExecStart=/c\\ExecStart=/usr/bin/valgrind -s --leak-check=yes "
                       f"--log-file={valgrind_log_path_controller} /usr/libexec/bluechi-controller' "
                       f"/usr/lib/systemd/system/bluechi-controller.service")
-        self.exec_run(f"sed -i '/ExecStart=/c\\ExecStart=/usr/bin/valgrind --leak-check=yes "
-                      f"--log-file={valgrind_log_path_controller} /usr/libexec/bluechi-agent' "
+        self.exec_run(f"sed -i '/ExecStart=/c\\ExecStart=/usr/bin/valgrind -s --leak-check=yes "
+                      f"--log-file={valgrind_log_path_agent} /usr/libexec/bluechi-agent' "
                       f"/usr/lib/systemd/system/bluechi-agent.service")
         self.exec_run("mkdir -p /var/log/valgrind")
         self.exec_run("systemctl daemon-reload")