Skip to content

fix(coro): revalidate task-group final wake - #1057

Merged
Coldwings merged 1 commit into
mainfrom
fix/task-group-stale-final
Aug 13, 2026
Merged

fix(coro): revalidate task-group final wake#1057
Coldwings merged 1 commit into
mainfrom
fix/task-group-stale-final

Conversation

@Coldwings

Copy link
Copy Markdown
Owner

Description

Fixes a task-group completion race in which an older 1 -> 0 child-count
transition could select a join waiter after another child had already been
accepted. That stale wake could let task_group::join() resume while the newer
child was still outstanding.

The completion waiter slot now supports conditional selection under the same
slot mutex used for waiter registration. Task-group completion revalidates that
the outstanding count is still zero through this operation before selecting the
join continuation.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)
  • Performance improvement (optimization that improves speed/memory usage)
  • Documentation (changes to documentation, comments, or examples)
  • Refactoring (code changes that neither fix bugs nor add features)
  • Tests (adding or modifying tests)
  • Build/CI (changes to build system, CI configuration, or dependencies)

Related Issues

Closes #1056
Related to #1049
Related to #1054

Changes Made

Core Changes

  • Added completion_waiter_slot::take_if() to evaluate wake eligibility while
    holding the waiter-slot mutex; the existing take() behavior is preserved as
    an unconditional wrapper.
  • Changed task-group final completion to select the join waiter only when the
    outstanding count remains zero under that same slot serialization.
  • Added a deterministic regression that pauses an old final transition,
    registers a new child and join waiter, and proves only the new child's true
    final transition can select and resume the waiter.
  • Added a low-level completion-waiter regression proving a false conditional
    selection leaves the original waiter linked for a later true selection.
  • Updated the task-group contract and changelog.

API Changes (if applicable)

No public API, ABI layout, or exception-specification change. take_if() is an
internal completion-waiter helper.

Migration Guide (if breaking change)

Not applicable.

Testing

Unit Tests

  • Added new tests for the changes
  • Updated existing tests if needed
  • All tests pass locally

Integration Tests

  • Tested with the existing full unit and integration test binary
  • Tested in real-world scenarios (not applicable to this internal race fix)

Sanitizer Testing

  • Tested with ASAN (AddressSanitizer)
  • Tested with TSAN (ThreadSanitizer)
  • No new warnings or errors

Test Results

Focused normal:
  stale-final regression: 9 assertions / 1 case passed
  completion-waiter state machine: 55 assertions / 1 case passed
  task_group suite: 146 assertions / 35 cases passed

Full normal:
  13,424 assertions / 834 cases passed

Full ASAN (detect_leaks=1, halt_on_error=1):
  13,426 assertions / 834 cases passed
  no ASAN, LeakSanitizer, or UBSan diagnostics

Full TSAN (halt_on_error=1, second_deadlock_stack=1):
  13,420 assertions passed
  834 cases = 833 passed + 1 existing fork-based TSAN skip
  no ThreadSanitizer diagnostics

All builds were out-of-source with cmake --build <build-dir> --parallel 2.

Checklist

Code Quality

  • My code follows the project's code style
  • I have added/updated comments for complex logic
  • I have removed any debug code, TODOs, or commented-out code
  • My changes generate no new warnings

Documentation

  • I have updated documentation (wiki, README, code comments)
  • I have added examples for new features (not applicable)
  • I have updated API documentation (if applicable)

Testing

  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested with ASAN and TSAN

Compatibility

  • My changes are backward compatible (or I've documented breaking changes)
  • I have considered the impact on existing users
  • I have updated CHANGELOG.md (if applicable)

Performance (if applicable)

  • I have considered the performance impact
  • I have added benchmarks for performance-critical changes (not applicable; the unconditional take() path remains a wrapper over the same locked selection)

Screenshots / Diagrams

Not applicable.

Additional Notes

The fix relies on the existing public task-group ordering: join() closes
admission under the group mutex before its waiter can be registered. Selection
and waiter registration then use the same slot-lock / outstanding-count order,
so the recheck neither introduces a lock-order cycle nor permits a future valid
admission after a registered waiter exists.

Reviewer Guidance

Areas requiring special attention:

  • The lock order between the completion-waiter slot and task-group child count.
  • The old-zero / new-admission / waiter-registration interleaving in the new
    deterministic regression.
  • Preservation of wake-lease claim, abandonment, move, and destruction behavior.

Questions for reviewers:

  • Does conditional selection close the stale-zero window without changing
    unconditional completion-waiter consumers?
  • Are all task-group waiter-registration and final-selection interleavings
    serialized without a missed wake or premature wake?

Thank you for contributing to Elio! 🎉

Copilot AI balanced review requested due to automatic review settings August 13, 2026 14:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a task-group race that could prematurely resume join() while a newly admitted child remained outstanding.

Changes:

  • Adds conditional waiter selection under the waiter-slot mutex.
  • Revalidates task-group completion before selecting the join continuation.
  • Adds deterministic regression tests and updates documentation.

Reviewed changes

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

Show a summary per file
File Description
include/elio/coro/detail/completion_waiter.hpp Adds conditional waiter selection.
include/elio/coro/task_group.hpp Revalidates final wake eligibility.
tests/unit/test_task.cpp Tests conditional waiter selection.
tests/unit/test_task_group.cpp Reproduces and verifies the race fix.
wiki/API-Contracts.md Documents task-group wake semantics.
CHANGELOG.md Records the bug fix.

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

@Coldwings
Coldwings merged commit cca748a into main Aug 13, 2026
11 checks passed
@Coldwings
Coldwings deleted the fix/task-group-stale-final branch August 13, 2026 15:10
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.

[Bug] task_group can join before a child accepted after an earlier zero transition

2 participants