Skip to content

Fix WhenAllTask constructor resetting _completedTasks counter#143

Open
YunchuWang wants to merge 2 commits intomainfrom
wangbill/fix-whenall-counter-reset
Open

Fix WhenAllTask constructor resetting _completedTasks counter#143
YunchuWang wants to merge 2 commits intomainfrom
wangbill/fix-whenall-counter-reset

Conversation

@YunchuWang
Copy link
Member

Problem

Fixes #131

The WhenAllTask constructor resets _completedTasks to 0 after calling super(tasks). The CompositeTask base constructor processes already-complete children via onChildCompleted(), correctly incrementing _completedTasks. The subsequent reset in WhenAllTask discards this count, causing the task to never complete when some children are already complete at construction time.

Root Cause

In when-all-task.ts, lines 14-15:

constructor(tasks: Task<T>[]) {
    super(tasks);           // CompositeTask processes pre-completed children → _completedTasks = K
    this._completedTasks = 0; // ← BUG: resets counter to 0
    this._failedTasks = 0;    // ← Dead code: never incremented anywhere
}

When WhenAllTask is constructed with a mix of already-complete and pending tasks:

  1. CompositeTask correctly counts the already-complete children (e.g., _completedTasks = K)
  2. WhenAllTask resets _completedTasks = 0
  3. When the remaining N - K tasks complete, _completedTasks reaches N - K instead of N
  4. The completion check _completedTasks == _tasks.length is never true
  5. The WhenAllTask never completes, causing the orchestration to hang

Fix

Removed the redundant this._completedTasks = 0 and this._failedTasks = 0 lines. The base class CompositeTask already initializes both fields to 0 before processing pre-completed children.

Tests

Added 8 unit tests in when-all-task.spec.ts:

All 839 unit tests pass.

Fixes #131

The WhenAllTask constructor redundantly re-initialized _completedTasks
and _failedTasks to 0 after calling super(tasks). Since CompositeTask's
constructor already initializes these fields and then processes
pre-completed children via onChildCompleted(), the reset wiped out
the correct count, causing WhenAllTask to never complete when some
children were already complete at construction time.

Also removes _failedTasks reset (dead code - never incremented anywhere).

Added 8 unit tests for WhenAllTask covering:
- Empty task array
- All pending children completing
- Fail-fast on child failure
- Pre-completed children (the bug scenario)
- All children pre-completed
- Pre-failed child
- Post-fail-fast completion
- Pending tasks count tracking
Copilot AI review requested due to automatic review settings March 6, 2026 00:42
…tion or class'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Copy link

Copilot AI left a comment

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 issue #131 where WhenAllTask's constructor reset _completedTasks to 0 after calling super(tasks), which caused WhenAllTask to never complete when constructed with pre-completed children. The CompositeTask base class constructor correctly initializes counters to 0 and then processes pre-completed children via onChildCompleted(). The redundant re-initialization in the subclass wiped out that count.

Changes:

  • Removed the redundant this._completedTasks = 0 and this._failedTasks = 0 lines from the WhenAllTask constructor and added an explanatory comment
  • Added a comprehensive test file (when-all-task.spec.ts) with 8 unit tests covering empty arrays, pending children, fail-fast, pre-completed children (the bug scenario), and pending tasks tracking

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/durabletask-js/src/task/when-all-task.ts Removed redundant counter re-initialization after super() call, added explanatory comment
packages/durabletask-js/test/when-all-task.spec.ts New test file with 8 unit tests covering WhenAllTask behavior including the regression scenario from #131


import { WhenAllTask } from "../src/task/when-all-task";
import { CompletableTask } from "../src/task/completable-task";

Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The Task import is unused in this test file — only WhenAllTask and CompletableTask are referenced. It should be removed to keep the imports clean.

Copilot uses AI. Check for mistakes.
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.

[copilot-finds] Bug: WhenAllTask constructor resets _completedTasks counter, causing hang with pre-completed children

2 participants