From 8931a8de72c434286d84f2dff52b29a5fa9a9d15 Mon Sep 17 00:00:00 2001
From: Tamaki Nishino <otamachan@gmail.com>
Date: Tue, 21 Apr 2026 23:48:55 +0900
Subject: [PATCH] Fix Task move-assignment to call request_stop before destroy

Task::operator=(Task&&) was destroying the coroutine handle without
calling request_stop() first, unlike the destructor which correctly
calls request_stop() before handle.destroy(). This meant that any
registered stop_callbacks (e.g. TimerStream cancel, GoalStream cancel)
would never fire during move-assignment, leaving dangling coroutine
handles that could be resumed after the frame was destroyed.

This caused SIGBUS/SIGSEGV crashes when a Task holding a coroutine
suspended on TimerStream::next() was overwritten via move-assignment
(e.g. `task = Task<void>{}`), because the timer callback would later
resume the destroyed coroutine handle.

The fix adds handle.promise().stop_source.request_stop() before
handle.destroy() in operator=(Task&&) for both Task<T> and Task<void>,
matching the existing destructor behavior.
---
 rclcpp_async/include/rclcpp_async/task.hpp |  2 +
 rclcpp_async/test/test_task.cpp            | 47 ++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/rclcpp_async/include/rclcpp_async/task.hpp b/rclcpp_async/include/rclcpp_async/task.hpp
index a3f4f42..6504630 100644
--- a/rclcpp_async/include/rclcpp_async/task.hpp
+++ b/rclcpp_async/include/rclcpp_async/task.hpp
@@ -145,6 +145,7 @@ struct Task
   {
     if (this != &o) {
       if (handle) {
+        handle.promise().stop_source.request_stop();
         handle.destroy();
       }
       handle = o.handle;
@@ -264,6 +265,7 @@ struct Task<void>
   {
     if (this != &o) {
       if (handle) {
+        handle.promise().stop_source.request_stop();
         handle.destroy();
       }
       handle = o.handle;
diff --git a/rclcpp_async/test/test_task.cpp b/rclcpp_async/test/test_task.cpp
index 5f7b603..92daece 100644
--- a/rclcpp_async/test/test_task.cpp
+++ b/rclcpp_async/test/test_task.cpp
@@ -312,3 +312,50 @@ TEST(TaskVoid, OperatorBoolAndDone)
   EXPECT_TRUE(static_cast<bool>(task2));
   EXPECT_TRUE(task2.done());
 }
+
+TEST(TaskT, MoveAssignRequestsStop)
+{
+  auto task = returns_42();
+  // Capture a token (not a reference to stop_source) so the shared stop-state
+  // outlives the coroutine frame destruction in move-assign.
+  auto token = task.handle.promise().stop_source.get_token();
+  EXPECT_FALSE(token.stop_requested());
+
+  // Move-assign with empty Task should request_stop on the old handle.
+  task = Task<int>{};
+  EXPECT_TRUE(token.stop_requested());
+}
+
+TEST(TaskVoid, MoveAssignRequestsStop)
+{
+  auto task = does_nothing();
+  auto token = task.handle.promise().stop_source.get_token();
+  EXPECT_FALSE(token.stop_requested());
+
+  task = Task<void>{};
+  EXPECT_TRUE(token.stop_requested());
+}
+
+TEST(TaskT, MoveAssignStopCallbackFires)
+{
+  bool callback_fired = false;
+  auto task = returns_42();
+  auto token = task.handle.promise().stop_source.get_token();
+  std::stop_callback cb(token, [&]() { callback_fired = true; });
+
+  EXPECT_FALSE(callback_fired);
+  task = Task<int>{};
+  EXPECT_TRUE(callback_fired);
+}
+
+TEST(TaskVoid, MoveAssignStopCallbackFires)
+{
+  bool callback_fired = false;
+  auto task = does_nothing();
+  auto token = task.handle.promise().stop_source.get_token();
+  std::stop_callback cb(token, [&]() { callback_fired = true; });
+
+  EXPECT_FALSE(callback_fired);
+  task = Task<void>{};
+  EXPECT_TRUE(callback_fired);
+}