ThreadPoolTest.NoWorkAfterShutdown hangs indefinitely under TSan

[mvillmow]

Summary

ThreadPoolTest.NoWorkAfterShutdown hangs indefinitely when run under ThreadSanitizer (TSan), causing the CI job to time out.

Test Location

tests/unit/test_thread_pool.cpp:133

Repro

make compile.debug.tsan.native
./build/x86.debug.tsan/bin/concurrency_unit_tests --gtest_filter=ThreadPoolTest.NoWorkAfterShutdown
# Hangs indefinitely

Root Cause (Suspected)

The test calls pool.submit() after pool.shutdown(). Under TSan's overhead, this appears to cause a deadlock. The likely issue is that TSan slows execution enough that a race condition in the ThreadPool's shutdown/submit path manifests as a hang.

Current Workaround

Test disabled via DISABLED_NoWorkAfterShutdown prefix.

Investigation Needed

1. Reproduce the hang under TSan locally
2. Identify whether the ThreadPool has a deadlock in the submit-after-shutdown path
3. Either fix the race or document why submit-after-shutdown is safe to be a no-op

🤖 Generated with Claude Code

[mvillmow]

Implementation Plan

Now I have a complete picture of the issue. Let me analyze the root cause and write the implementation plan.

Root Cause Analysis:

The NoWorkAfterShutdown test does:

1. Create pool (spawns worker threads)
2. Call pool.shutdown() — sets flag, notifies, joins threads
3. Call pool.submit() after shutdown
4. Sleep 100ms
5. Assert counter == 0

The hang is in pool.shutdown() itself, specifically in worker.join(). Here's the race:

• shutdown() stores true into shutdown_requested_ (atomic, no lock)
• Then calls condition_.notify_all()
• Worker threads are waiting on condition_.wait(lock, predicate)
• The predicate reads shutdown_requested_.load() — but there's no mutex held when shutdown_requested_.store(true) happens
• Under TSan's slower execution, a worker thread can be between waking up from wait() and re-acquiring the lock when notify_all() fires. The worker then re-evaluates the predicate (sees shutdown=true, queue=empty) and returns. This part is fine.
• The actual problem: shutdown_requested_.store(true) happens outside the queue_mutex_. If a worker thread is in the middle of executing work (outside the lock), it will loop back, re-acquire the lock, wait again — and notify_all() has already fired before this. The worker is now stuck waiting with no one to wake it, and shutdown() is blocked on worker.join().

The fix: acquire queue_mutex_ before setting shutdown_requested_ and calling notify_all().

---

Implementation Plan: Fix ThreadPoolTest.NoWorkAfterShutdown TSan Hang (Issue #145)

1. Objective

Fix the deadlock in ThreadPool::shutdown() that causes ThreadPoolTest.NoWorkAfterShutdown to hang indefinitely under ThreadSanitizer, then re-enable the test.

---

2. Approach

Root Cause: shutdown() sets shutdown_requested_ = true and calls condition_.notify_all() without holding queue_mutex_. A worker thread that is executing work (outside the lock) will loop back, acquire the lock, and call condition_.wait() after notify_all() has already fired. The worker then blocks permanently, and shutdown() deadlocks on worker.join().

Under normal execution this rarely manifests because timing works out; under TSan's instrumentation overhead, the window is wide enough to reliably trigger the hang.

Fix: Hold queue_mutex_ while setting shutdown_requested_ and issuing notify_all(). This ensures any worker that re-acquires the lock post-execution will see the shutdown flag correctly without missing the notification — the standard condition_variable contract requires the flag mutation to be under the mutex that protects the predicate.

Test: Re-enable DISABLED_NoWorkAfterShutdown once the fix is in place. Remove the sleep_for(100ms) busy-wait; replace with a proper shutdown() + assert pattern that doesn't rely on timing.

---

3. Files to Create

None — this is a pure fix to existing files.

---

4. Files to Modify

src/concurrency/thread_pool.cpp

Change shutdown() — lines 47–62:

Current (broken):

void ThreadPool::shutdown() {
  shutdown_requested_.store(true);   // ← outside mutex
  condition_.notify_all();           // ← may fire before workers re-wait
  for (auto& worker : workers_) {
    if (worker.joinable()) { worker.join(); }
  }
  workers_.clear();
}

Fixed:

void ThreadPool::shutdown() {
  {
    std::lock_guard<std::mutex> lock(queue_mutex_);  // ← hold mutex
    shutdown_requested_.store(true);                  // ← flag set under mutex
  }                                                   // ← release before notify
  condition_.notify_all();                            // ← workers can't miss this
  for (auto& worker : workers_) {
    if (worker.joinable()) { worker.join(); }
  }
  workers_.clear();
}

The mutex is released before notify_all() to avoid waking a worker that immediately blocks trying to re-acquire the same mutex — this is the standard notify pattern. The critical invariant is that shutdown_requested_ is set while the mutex is held, so any worker that calls wait() after this point will immediately evaluate the predicate as true without blocking.

No change needed to worker_loop() — it already correctly reads shutdown_requested_ inside the unique_lock on queue_mutex_, which is the right pattern.

No change needed to submit() — it already reads shutdown_requested_ inside lock_guard<queue_mutex_>.

tests/unit/test_thread_pool.cpp

Change 1 — Re-enable the test (line 137):

Remove DISABLED_ prefix:

// Before:
TEST(ThreadPoolTest, DISABLED_NoWorkAfterShutdown) {

// After:
TEST(ThreadPoolTest, NoWorkAfterShutdown) {

Change 2 — Remove the comment referencing the disabled state (line 135–136):

// Before:
// Test: No new work accepted after shutdown
// DISABLED under TSan: hangs indefinitely (see issue #145)

// After:
// Test: No new work accepted after shutdown

Change 3 — Remove the racy sleep_for (lines 146–147):

The test currently waits 100ms after submit() to see if the task runs. Since shutdown() already joined all workers, there are no threads to run submitted work — the sleep_for is unnecessary and fragile. The correct assertion is immediate:

// Before:
pool.shutdown();
pool.submit([&]() { counter.fetch_add(1); });
std::this_thread::sleep_for(std::chrono::milliseconds(100));
EXPECT_EQ(counter.load(), 0);

// After:
pool.shutdown();
pool.submit([&]() { counter.fetch_add(1); });
// No sleep needed: submit() after shutdown() is a guaranteed no-op.
// Workers are already joined; no thread can execute the submitted item.
EXPECT_EQ(counter.load(), 0);

---

5. Implementation Order

1. Fix shutdown() in src/concurrency/thread_pool.cpp — add lock_guard<queue_mutex_> around the shutdown_requested_.store(true) call.

2. Update tests/unit/test_thread_pool.cpp — remove DISABLED_ prefix, remove disabled comment, remove sleep_for(100ms).

3. Build and run under TSan locally:

   cmake --preset tsan && cmake --build --preset tsan
   ctest --preset tsan --output-on-failure --timeout 120 \
         -R ThreadPoolTest.NoWorkAfterShutdown

4. Run the full TSan test suite to confirm no regressions:

   ctest --preset tsan --output-on-failure --timeout 120

5. Run ASan and debug builds as a sanity check:

   cmake --preset asan && cmake --build --preset asan
   ctest --preset asan --output-on-failure --timeout 120
   cmake --preset debug && cmake --build --preset debug
   ctest --preset debug --output-on-failure

6. Check formatting and linting:

   just format-check
   just lint

---

6. Verification

Check	Command	Expected
TSan — target test	ctest --preset tsan -R NoWorkAfterShutdown --timeout 120	PASS, no hang
TSan — full suite	ctest --preset tsan --output-on-failure --timeout 120	All pass
ASan — full suite	ctest --preset asan --output-on-failure --timeout 120	All pass
Debug — full suite	ctest --preset debug --output-on-failure	All pass
Format	just format-check	No violations
Lint	just lint	No warnings

A successful run means NoWorkAfterShutdown completes in well under 1 second (it should return almost immediately after pool.submit() since no workers exist).

---

7. Skills Used

Invoked during planning:

• hephaestus:advise (implicitly via the Prior Learnings section in the issue) — surfaced the cpp-ci-sanitizer-tsan-data-race-fixes plugin.

Team knowledge base skills referenced (from Prior Learnings):

• cpp-ci-sanitizer-tsan-data-race-fixes — directly described this exact test hang, the DISABLED_ pattern, the ctest --timeout 120 recommendation, and the general investigation approach for TSan data-race/deadlock issues. Key inputs used:
  • Confirmed DISABLED_NoWorkAfterShutdown as the correct disable pattern
  • Confirmed ctest --output-on-failure --timeout 120 as the right verification command
  • Guided investigation toward submit-after-shutdown deadlock as the root cause

---

Generated by Claude Code Planner

[mvillmow]

Created 4 follow-up issue(s): #321, #322, #323, #324