Skip to content

refactor: remove dead _failedTasks field and unused async-pageable.ts#284

Open
YunchuWang wants to merge 1 commit into
mainfrom
yunchuwang-remove-dead-code-composite-task
Open

refactor: remove dead _failedTasks field and unused async-pageable.ts#284
YunchuWang wants to merge 1 commit into
mainfrom
yunchuwang-remove-dead-code-composite-task

Conversation

@YunchuWang

Copy link
Copy Markdown
Member

Summary

Dead-code cleanup in the core SDK. No behavior change — build, lint, and the full core test suite (1051 tests) all pass.

What & why

1. Removed the _failedTasks field from CompositeTask

_failedTasks was declared and initialized to 0 in the constructor but never incremented or read anywhere in the repository. I confirmed this with a repo-wide search — the only occurrences were the declaration, the constructor init, and a comment:

packages/durabletask-js/src/task/composite-task.ts:12:  _failedTasks: number;
packages/durabletask-js/src/task/composite-task.ts:19:    this._failedTasks = 0;
packages/durabletask-js/src/task/when-all-task.ts:14:  // ...references _failedTasks

Neither WhenAllTask.onChildCompleted() nor WhenAnyTask.onChildCompleted() ever touched it. The field was misleading: it implied failed children were tracked separately, when in reality WhenAllTask counts every settled child — successful and failed — in the single _completedTasks counter (it increments before the fail-fast check). A separate failure counter was never needed.

2. Fixed the stale comment in WhenAllTask

The constructor comment warned against re-initializing _completedTasks or _failedTasks. With _failedTasks gone, the comment now references only _completedTasks so it stays accurate.

3. Deleted the dead src/utils/async-pageable.ts (91 lines)

This file contained an AsyncPageable class / Page interface / PageFetcher type that was never imported by any file. The real pagination implementation lives in src/orchestration/page.ts, which is exported from index.ts and consumed by client.ts and the export-history client. Verified before deleting:

  • No file references it by path (grep "async-pageable" → only the file itself).
  • Every AsyncPageable / createAsyncPageable import resolves to orchestration/page.ts (note: async-pageable.ts didn't even export createAsyncPageable).
  • It was not re-exported from any barrel (there is no utils/index.ts).

The core build stays green after deletion, confirming nothing depended on it.

4. Added tests documenting the real behavior

To capture the intent that _failedTasks misleadingly implied was separate:

  • when-all-task.spec.ts — verifies completedTasks counts both a successful child and a failed child (single counter, no separate failure count).
  • when-any-task.spec.ts — verifies WhenAnyTask settles on the first completed child regardless of success/failure and does not track failures separately (a later successful child does not override an already-settled failed child).

Verification

  • npm run build:core — clean compile (confirms deleting async-pageable.ts broke nothing).
  • npx eslint on all changed files — exit 0.
  • npx jest --runInBand (full core suite) — 59 suites, 1051 tests, all pass (includes the 2 new tests).

Fixes #217

The `_failedTasks` field on `CompositeTask` was declared and initialized to
0 but never incremented or read anywhere in the repo (confirmed by grep).
Neither `WhenAllTask.onChildCompleted()` nor `WhenAnyTask.onChildCompleted()`
touched it. It misleadingly implied failed children were tracked separately
from `_completedTasks`, when in fact `WhenAllTask` counts every settled child
-- successful or failed -- in the single `_completedTasks` counter. Removed
the field and updated the now-stale comment in `WhenAllTask` that referenced it.

`src/utils/async-pageable.ts` was an unimported `AsyncPageable` class
superseded by the real pagination implementation in `src/orchestration/page.ts`
(exported from index.ts and used by client.ts and the export-history client).
No file referenced it by path or symbol, and it was not re-exported from any
barrel, so deleting it is safe -- the core build stays green.

Added unit tests documenting the real behavior: `WhenAllTask.completedTasks`
counts both successful and failed child completions, and `WhenAnyTask` settles
on the first completed child regardless of success/failure without tracking
failures separately. No behavior change.

Fixes #217

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 7, 2026 21:42

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

Dead-code cleanup in the core DurableTask JS SDK to reduce confusion and better document actual composite task semantics, without changing runtime behavior.

Changes:

  • Removed the unused _failedTasks field from CompositeTask and updated the related stale comment in WhenAllTask.
  • Deleted the unused src/utils/async-pageable.ts implementation (pagination is provided by src/orchestration/page.ts).
  • Added unit tests to document that WhenAllTask.completedTasks counts both successful and failed child completions, and that WhenAnyTask settles on the first completed (even failed) child without later override.

Reviewed changes

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

Show a summary per file
File Description
packages/durabletask-js/src/task/composite-task.ts Removes dead _failedTasks field from composite task state.
packages/durabletask-js/src/task/when-all-task.ts Updates constructor comment to match current composite task behavior.
packages/durabletask-js/src/utils/async-pageable.ts Deletes unused pageable implementation to avoid duplicate/confusing pagination code.
packages/durabletask-js/test/when-all-task.spec.ts Adds regression test documenting completedTasks increments for both success and failure.
packages/durabletask-js/test/when-any-task.spec.ts Adds regression test documenting first-settled-child behavior even when the first completion is a failure.

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] Improve: Dead _failedTasks field in CompositeTask and dead async-pageable.ts file

2 participants