From f412699e4770ecfa181d90149029bb5c8f7b5657 Mon Sep 17 00:00:00 2001 From: wangbill Date: Tue, 7 Jul 2026 14:41:47 -0700 Subject: [PATCH] refactor: remove dead _failedTasks field and unused async-pageable.ts 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> --- .../durabletask-js/src/task/composite-task.ts | 2 - .../durabletask-js/src/task/when-all-task.ts | 4 +- .../src/utils/async-pageable.ts | 91 ------------------- .../durabletask-js/test/when-all-task.spec.ts | 21 +++++ .../durabletask-js/test/when-any-task.spec.ts | 21 +++++ 5 files changed, 44 insertions(+), 95 deletions(-) delete mode 100644 packages/durabletask-js/src/utils/async-pageable.ts diff --git a/packages/durabletask-js/src/task/composite-task.ts b/packages/durabletask-js/src/task/composite-task.ts index ac95c7b..022b254 100644 --- a/packages/durabletask-js/src/task/composite-task.ts +++ b/packages/durabletask-js/src/task/composite-task.ts @@ -9,14 +9,12 @@ import { Task } from "./task"; export class CompositeTask extends Task { _tasks: Task[] = []; _completedTasks: number; - _failedTasks: number; constructor(tasks: Task[]) { super(); this._tasks = tasks; this._completedTasks = 0; - this._failedTasks = 0; for (const task of tasks) { task._parent = this; diff --git a/packages/durabletask-js/src/task/when-all-task.ts b/packages/durabletask-js/src/task/when-all-task.ts index dc7ec46..8851a96 100644 --- a/packages/durabletask-js/src/task/when-all-task.ts +++ b/packages/durabletask-js/src/task/when-all-task.ts @@ -11,8 +11,8 @@ export class WhenAllTask extends CompositeTask { constructor(tasks: Task[]) { super(tasks); - // Note: Do NOT re-initialize _completedTasks or _failedTasks here. - // CompositeTask's constructor already initializes them to 0 and then + // Note: Do NOT re-initialize _completedTasks here. + // CompositeTask's constructor already initializes it to 0 and then // processes pre-completed children via onChildCompleted(), which // increments the counter. Re-initializing would wipe out that count // and cause the task to hang when some children are already complete. diff --git a/packages/durabletask-js/src/utils/async-pageable.ts b/packages/durabletask-js/src/utils/async-pageable.ts deleted file mode 100644 index e43b7cb..0000000 --- a/packages/durabletask-js/src/utils/async-pageable.ts +++ /dev/null @@ -1,91 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -/** - * Represents a page of results from a paginated query. - */ -export interface Page { - /** The values in this page. */ - values: T[]; - /** The continuation token for fetching the next page, or undefined if this is the last page. */ - continuationToken?: string; -} - -/** - * A function that fetches a page of results. - * @param continuationToken - The continuation token from the previous page, or undefined for the first page. - * @returns A promise that resolves to the next page of results. - */ -export type PageFetcher = (continuationToken?: string) => Promise>; - -/** - * Represents an asynchronous pageable collection that supports both - * item-by-item iteration and page-by-page iteration. - * - * This is similar to .NET's AsyncPageable from Azure SDK. - * - * @example - * // Iterate by items - * for await (const entity of client.getEntities(query)) { - * console.log(entity.id); - * } - * - * @example - * // Iterate by pages - * for await (const page of client.getEntities(query).byPage()) { - * console.log(`Got ${page.values.length} items`); - * for (const entity of page.values) { - * console.log(entity.id); - * } - * } - */ -export class AsyncPageable { - private readonly fetchPage: PageFetcher; - - /** - * Creates a new AsyncPageable instance. - * @param fetchPage - A function that fetches a page of results given a continuation token. - */ - constructor(fetchPage: PageFetcher) { - this.fetchPage = fetchPage; - } - - /** - * Creates an AsyncPageable from a page fetcher function. - * @param fetchPage - A function that fetches a page of results. - * @returns An AsyncPageable instance. - */ - static create(fetchPage: PageFetcher): AsyncPageable { - return new AsyncPageable(fetchPage); - } - - /** - * Implements the async iterator protocol to iterate over individual items. - * This automatically handles pagination, fetching additional pages as needed. - */ - async *[Symbol.asyncIterator](): AsyncGenerator { - for await (const page of this.byPage()) { - for (const item of page.values) { - yield item; - } - } - } - - /** - * Returns an async generator that yields pages of results. - * Use this when you need access to page boundaries or continuation tokens. - * - * @param options - Optional settings for page iteration. - * @param options.continuationToken - A continuation token to resume from a specific page. - * @returns An async generator that yields pages. - */ - async *byPage(options?: { continuationToken?: string }): AsyncGenerator, void, unknown> { - let continuationToken: string | undefined = options?.continuationToken; - - do { - const page = await this.fetchPage(continuationToken); - yield page; - continuationToken = page.continuationToken; - } while (continuationToken); - } -} diff --git a/packages/durabletask-js/test/when-all-task.spec.ts b/packages/durabletask-js/test/when-all-task.spec.ts index a9e4b97..bddded9 100644 --- a/packages/durabletask-js/test/when-all-task.spec.ts +++ b/packages/durabletask-js/test/when-all-task.spec.ts @@ -126,4 +126,25 @@ describe("WhenAllTask", () => { child3.complete(3); expect(task.pendingTasks()).toBe(0); }); + + // Issue #217: failed child completions are counted in the same _completedTasks + // counter as successful ones. There is no separate failed-task counter, so a + // failing child increments completedTasks just like a successful one before + // fail-fast completion kicks in. + it("should count both successful and failed children in completedTasks", () => { + const child1 = new CompletableTask(); + const child2 = new CompletableTask(); + const child3 = new CompletableTask(); + const task = new WhenAllTask([child1, child2, child3]); + + child1.complete(1); + expect(task.completedTasks).toBe(1); + + // The failing child increments the same counter before fail-fast completes the task. + child2.fail("child failed"); + + expect(task.completedTasks).toBe(2); + expect(task.isComplete).toBe(true); + expect(task.isFailed).toBe(true); + }); }); diff --git a/packages/durabletask-js/test/when-any-task.spec.ts b/packages/durabletask-js/test/when-any-task.spec.ts index ff1fc57..9f95875 100644 --- a/packages/durabletask-js/test/when-any-task.spec.ts +++ b/packages/durabletask-js/test/when-any-task.spec.ts @@ -148,4 +148,25 @@ describe("WhenAnyTask", () => { expect(task.isComplete).toBe(false); expect(task.isFailed).toBe(false); }); + + // Issue #217: a failed completion settles WhenAny immediately and is treated the + // same as a successful one — failures are not tracked separately, so a later + // successful child does not override the already-settled failed child. + it("should keep the first failed child as the result even if a later child succeeds", () => { + const child1 = new CompletableTask(); + const child2 = new CompletableTask(); + const task = new WhenAnyTask([child1, child2]); + + child1.fail("first failed"); + + expect(task.isComplete).toBe(true); + // WhenAny itself is not failed — it simply returns the settled (failed) child. + expect(task.isFailed).toBe(false); + expect(task.getResult()).toBe(child1); + + // A subsequent successful completion must not replace the already-settled failed child. + child2.complete(99); + expect(task.getResult()).toBe(child1); + expect(task.getResult().isFailed).toBe(true); + }); });