task: let the TaskState instance registry outlive static destruction

~TaskState() takes s_instance_mutex, asserts on s_instance_map[m_id] and
erases from it.  Both were ordinary file-scope statics, so both are
destroyed when main() returns -- and a TaskState can be destroyed long
after that, by a thread that has no idea main() is gone.

TaskConsumer threads are detached and never joined.  TaskMasterState is
owned by shared_ptrs those threads hold, so whichever thread drops the
last reference runs ~TaskMasterState, and with it the destructors of
every TaskState still queued.  That thread is routinely a consumer, not
main.  Instrumented runs show the ordering directly:

    tid=2146948  ~TaskMaster           <- on a consumer thread
    tid=2146940  STATIC_DTORS_BEGIN    <- main starts static destruction
    tid=2146948  ~TaskState            <- 3.4 us later, touches the map

A destroyed std::map is worse than stale.  erase() frees nodes its own
destructor already freed, and operator[] inserts into freed storage, so
the failure is a heap-use-after-free or a double-free rather than a
missing entry -- which is what AddressSanitizer reports, always in a
worker thread, always after the last test has passed.

Make the mutex and the map references to deliberately leaked objects.
Nothing frees them, so nothing can use them after they are freed.  Every
call site is unchanged, and the cost is one pointer load where there was
none: holding them behind a shared_ptr would give the same lifetime
guarantee, but would put atomic refcount traffic on TaskState
construction and destruction, which is the one path a task system cannot
afford it on.  s_instance_count needs no such treatment and gets a
static_assert saying why: it is trivially destructible, so its storage
stays valid and nothing is freed.

Leaking at exit costs nothing here.  The daemon does not run destructors
at all -- BeesContext::stop_running() ends in _exit(EXIT_SUCCESS), "skip
all destructors, do not pass GO" -- so this only ever mattered to
programs that return from main(), which is to say the test suite.

Measured with an AddressSanitizer build of test-bees-borrower under
8-way concurrent load, which is what the race needs (it does not
reproduce serially): 47 reports in 3000 runs before, 0 in 3000 after.

Assisted-by: Claude-Code:claude-opus-5

diff --git a/lib/task.cc b/lib/task.cc
index ce6bf27089502f1873c777769d8edceb9fa8ec43..7d50d6277dd03cb4b224eb9a288bd9fb0391ac28 100644 (file)
--- a/lib/task.cc
+++ b/lib/task.cc
@@ -101,8 +101,9 @@ namespace crucible {
                static atomic<size_t>                   s_instance_count;
 
                /// All currently existing tasks
-               static mutex                            s_instance_mutex;
-               static map<TaskId, TaskState*>  s_instance_map;
+               // References to deliberately-leaked objects: see the definitions.
+               static mutex                            &s_instance_mutex;
+               static map<TaskId, TaskState*>  &s_instance_map;
 
                /// Identifier for this task
                TaskId                                  m_id;
@@ -191,8 +192,25 @@ namespace crucible {
 
        atomic<TaskId> TaskState::s_next_id;
        atomic<size_t> TaskState::s_instance_count;
-       mutex TaskState::s_instance_mutex;
-       map<TaskId, TaskState*> TaskState::s_instance_map;
+
+       // ~TaskState() reaches into the mutex and the map below, and a TaskState
+       // can be destroyed by any thread that happens to drop its last reference.
+       // TaskConsumer threads are detached and never joined, and TaskMasterState
+       // is owned by shared_ptrs those threads hold, so ~TaskMasterState -- and
+       // every TaskState it still owns -- can run on a consumer thread after
+       // main() has returned and static destruction has begun.  A destroyed
+       // std::map is not merely stale: erase() frees nodes its destructor already
+       // freed, and operator[] inserts into freed storage.
+       //
+       // So these two outlive everything.  Leaking them at exit costs nothing:
+       // the process is going away, and the daemon already skips destructors
+       // entirely (bees-context.cc calls _exit()).  The counter above needs no
+       // such treatment -- it is trivially destructible, so its storage stays
+       // valid for the whole process lifetime and nothing is freed.
+       static_assert(is_trivially_destructible<atomic<size_t>>::value,
+               "s_instance_count must be trivially destructible to be used during static destruction");
+       mutex &TaskState::s_instance_mutex = *new mutex();
+       map<TaskId, TaskState*> &TaskState::s_instance_map = *new map<TaskId, TaskState*>();
 
        class TaskMasterState : public enable_shared_from_this<TaskMasterState> {
                mutex                                   m_mutex;