Skip to content

Restore mutex-protected worker shutdown tracking - #679

Merged
simsong merged 2 commits into
mainfrom
codex/issue-678-threadpool-shutdown
Aug 14, 2026
Merged

Restore mutex-protected worker shutdown tracking#679
simsong merged 2 commits into
mainfrom
codex/issue-678-threadpool-shutdown

Conversation

@simsong-codex

@simsong-codex simsong-codex commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • restore the live-worker set as the authoritative shutdown predicate
  • protect worker registration, removal, observation, and join predicates with the pool mutex
  • retain explicit std::thread::join() calls and make worker/thread registration rollback-safe
  • add 25 timed shutdown attempts with 32 idle workers per attempt
  • document the user-visible shutdown fix in the 2.2.0 release notes
  • prepare this open PR head as the v2.2.0beta2 release candidate tracked in release: v2.2.0beta2 #680

Root cause

The atomic workers_running counter was changed and followed by a condition-variable notification without holding the mutex used by thread_pool::join(). With multiple workers exiting, the last notification could occur after the join predicate was checked but before the joining thread actually slept. The counter could then be zero with no future notifier, leaving the timed join asleep until its deadline.

The restored worker set closes that window because worker removal and workers.empty() are ordered by the same mutex used by TO_MAIN.

Addresses #678.

Validation

  • make -C src check TESTS=test_be20_api — pass
  • make check — pass (4/4 test programs; 0 failures, 0 errors)
  • make distcheck — pass (clean archive build, install/uninstall, and 4/4 test programs)
  • GitHub Actions — pass on current head cbc2cc994d9b47330e27b582947e9a40d82c738b, including macOS, Ubuntu, AddressSanitizer, Windows runtime, MinGW, Codecov, documentation, Snap, and CodeQL
  • Current-head Copilot review — pass on cbc2cc994d9b47330e27b582947e9a40d82c738b; 5/5 changed files reviewed with no new comments and no review threads

Beta test release

The signed, reviewed open-PR head is published as
v2.2.0beta2.
PR #679 remains open and unmerged while beta feedback is collected.

@simsong-codex
simsong-codex marked this pull request as ready for review August 13, 2026 11:16
@simsong-codex
simsong-codex requested a review from simsong as a code owner August 13, 2026 11:16
@simsong-codex
simsong-codex marked this pull request as draft August 13, 2026 11:16
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.79%. Comparing base (586a75a) to head (cbc2cc9).

Files with missing lines Patch % Lines
src/be20_api/threadpool.cpp 77.78% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
+ Coverage   77.78%   77.79%   +0.02%     
==========================================
  Files         118      118              
  Lines       14332    14361      +29     
==========================================
+ Hits        11147    11172      +25     
- Misses       3185     3189       +4     
Flag Coverage Δ
unittests 77.79% <88.89%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@simsong-codex
simsong-codex marked this pull request as ready for review August 13, 2026 11:33
@simsong-codex
simsong-codex marked this pull request as draft August 13, 2026 11:33
@simsong
simsong marked this pull request as ready for review August 13, 2026 11:53
Copilot AI lite review requested due to automatic review settings August 13, 2026 11:53
simsong
simsong previously approved these changes Aug 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an intermittent thread_pool::join() stall by restoring a mutex-ordered shutdown predicate based on a live-worker set, eliminating a lost-wakeup window that could leave timed joins sleeping until their deadline even after all workers had exited (Issue #678).

Changes:

  • Replace the atomic “workers running” counter with a mutex-protected live workers set and use workers.empty() as the shutdown/join predicate.
  • Make worker/thread registration rollback-safe and keep explicit std::thread::join() behavior.
  • Add a regression test that repeatedly exercises timed idle-worker shutdown, and document the user-visible fix in the 2.2.0 release notes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/be20_api/threadpool.h Introduces a mutex-protected live-worker set to serve as the authoritative shutdown predicate.
src/be20_api/threadpool.cpp Implements mutex-ordered worker registration/removal and updates join predicates to use workers.empty().
src/be20_api/test_be20_threadpool.cpp Adds a repeated timed-join regression test to detect the prior idle-worker shutdown stall.
doc/RELEASE_NOTES.md Documents the shutdown/join stall fix as a user-visible change for 2.2.0.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@simsong
simsong merged commit aa5b136 into main Aug 14, 2026
27 checks passed
@simsong
simsong deleted the codex/issue-678-threadpool-shutdown branch August 14, 2026 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants