diff --git a/CHANGELOG.md b/CHANGELOG.md index b2d6d9f..331be25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,9 @@ ### Fixes +- Prevent shared or duplicate pending tasks from stranding `whenAll` orchestrations after all + activities finish. Notify every dependent composite, preserve duplicate result positions, + and detach completed `whenAny` groups from their remaining children. - Preserve trailing external events in the same work item after `continueAsNew(..., true)`, rather than delivering them to abandoned listeners in the previous execution. Already-consumed events are not duplicated; `saveEvents=false` still discards unprocessed events. Case-insensitive diff --git a/README.md b/README.md index 0ebb0a4..823f6de 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,9 @@ a sibling's completion callback. Do not yield a canceled timer expecting success If that callback throws, do not catch it and reuse the `whenAll` group or its parents: the group can already be marked complete without a result and without notifying its parent. Like Python, `getResult()` rejects this uninitialized result instead of treating it as success. -Parent notification is not resumed after the callback exception. +That group does not notify its own parents, but other groups observing the same child +are still notified. If multiple completion callbacks throw, their errors are surfaced +together in an `AggregateError`. Custom `Task` subclasses now have their completed `getResult()` accessor called on each yield instead of reading the raw result field. Accessors must be replay-safe; thrown errors fail execution. @@ -294,6 +296,10 @@ const orchestrator: TOrchestrator = async function* (ctx: OrchestrationContext): You can find the full sample at [examples/hello-world/fanout-fanin.ts](./examples/hello-world/fanout-fanin.ts). +Tasks can be shared between `whenAll` and `whenAny` groups without scheduling the activity again. +`whenAll` preserves every input position: `whenAll([task, task])` returns the task's result twice, +whether the task is pending or already complete. + ### Human interaction and durable timers An orchestration can wait for external events, such as a human approval, with optional timeout handling: diff --git a/packages/durabletask-js/src/task/completable-task.ts b/packages/durabletask-js/src/task/completable-task.ts index 4d82d3d..ebeb299 100644 --- a/packages/durabletask-js/src/task/completable-task.ts +++ b/packages/durabletask-js/src/task/completable-task.ts @@ -18,9 +18,7 @@ export class CompletableTask extends Task { this._result = result; this._isComplete = true; - if (this._parent) { - this._parent.onChildCompleted(this); - } + this.notifyParents(); } fail(message: string, details?: pb.TaskFailureDetails): void { @@ -33,9 +31,7 @@ export class CompletableTask extends Task { this._exception = new TaskFailedError(message, details); this._isComplete = true; - if (this._parent) { - this._parent.onChildCompleted(this); - } + this.notifyParents(); } /** @@ -51,8 +47,6 @@ export class CompletableTask extends Task { this._exception = error; this._isComplete = true; - if (this._parent) { - this._parent.onChildCompleted(this); - } + this.notifyParents(); } } diff --git a/packages/durabletask-js/src/task/composite-task.ts b/packages/durabletask-js/src/task/composite-task.ts index 022b254..19556bf 100644 --- a/packages/durabletask-js/src/task/composite-task.ts +++ b/packages/durabletask-js/src/task/composite-task.ts @@ -9,6 +9,7 @@ import { Task } from "./task"; export class CompositeTask extends Task { _tasks: Task[] = []; _completedTasks: number; + protected readonly _pendingChildren = new Map, number>(); constructor(tasks: Task[]) { super(); @@ -17,14 +18,26 @@ export class CompositeTask extends Task { this._completedTasks = 0; for (const task of tasks) { - task._parent = this; + this._pendingChildren.set(task, (this._pendingChildren.get(task) ?? 0) + 1); + } - if (task._isComplete) { + for (const task of this._pendingChildren.keys()) { + if (task.isComplete) { this.onChildCompleted(task); + } else { + task._parents.add(this); } } } + protected override notifyParents(): void { + for (const task of this._pendingChildren.keys()) { + task._parents.delete(this); + } + this._pendingChildren.clear(); + super.notifyParents(); + } + // @todo: should be abstract method onChildCompleted(_: Task): void {} } diff --git a/packages/durabletask-js/src/task/index.ts b/packages/durabletask-js/src/task/index.ts index c522797..2ed7626 100644 --- a/packages/durabletask-js/src/task/index.ts +++ b/packages/durabletask-js/src/task/index.ts @@ -6,10 +6,11 @@ import { Task } from "./task"; import { WhenAnyTask } from "./when-any-task"; /** - * Returns a task that completes when all of the provided tasks complete or when one of the tasks fail + * Returns a task that completes after every provided task completes or fails. + * Results preserve input order and duplicate task references. Tasks may be shared between groups. * * @param tasks the tasks to wait for - * @returns {WhenAllTask} a task that completes when all of the provided tasks complete or when one of the tasks fail + * @returns {WhenAllTask} a task containing the ordered results, or an aggregate of child failures */ export function whenAll(tasks: Task[]): WhenAllTask { return new WhenAllTask(tasks); diff --git a/packages/durabletask-js/src/task/retry-task-base.ts b/packages/durabletask-js/src/task/retry-task-base.ts index 7e82664..891dcc0 100644 --- a/packages/durabletask-js/src/task/retry-task-base.ts +++ b/packages/durabletask-js/src/task/retry-task-base.ts @@ -33,11 +33,7 @@ export abstract class RetryTaskBase extends CompletableTask { * @param startTime - The time when the task was first scheduled * @param taskType - The type of task (activity or sub-orchestration) */ - constructor( - action: pb.OrchestratorAction, - startTime: Date, - taskType: RetryTaskType, - ) { + constructor(action: pb.OrchestratorAction, startTime: Date, taskType: RetryTaskType) { super(); this._action = action; this._startTime = startTime; @@ -151,8 +147,6 @@ export abstract class RetryTaskBase extends CompletableTask { this._exception = new TaskFailedError(message, details); this._isComplete = true; - if (this._parent) { - this._parent.onChildCompleted(this); - } + this.notifyParents(); } } diff --git a/packages/durabletask-js/src/task/task.ts b/packages/durabletask-js/src/task/task.ts index ba709be..dacb2d1 100644 --- a/packages/durabletask-js/src/task/task.ts +++ b/packages/durabletask-js/src/task/task.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { CompositeTask } from "./composite-task"; +import type { CompositeTask } from "./composite-task"; /** * Abstract base class for asynchronous tasks in a durable orchestration. @@ -9,13 +9,12 @@ import { CompositeTask } from "./composite-task"; export class Task { _result: T | undefined; _exception: Error | undefined; - _parent: CompositeTask | undefined; + readonly _parents = new Set>(); _isComplete: boolean = false; constructor() { this._isComplete = false; this._exception = undefined; - this._parent = undefined; } /** @@ -83,4 +82,26 @@ export class Task { return this._exception; } + + protected notifyParents(): void { + const parents = [...this._parents]; + this._parents.clear(); + const errors: unknown[] = []; + + // A canceled result can throw in one callback without preventing the other observers from completing. + for (const parent of parents) { + try { + parent.onChildCompleted(this); + } catch (error) { + errors.push(error); + } + } + + if (errors.length === 1) { + throw errors[0]; + } + if (errors.length > 1) { + throw new AggregateError(errors, "Multiple task completion callbacks failed"); + } + } } diff --git a/packages/durabletask-js/src/task/timer-task.ts b/packages/durabletask-js/src/task/timer-task.ts index 23268cb..54bc640 100644 --- a/packages/durabletask-js/src/task/timer-task.ts +++ b/packages/durabletask-js/src/task/timer-task.ts @@ -82,12 +82,12 @@ export class TimerTask extends CompletableTask { * remains for it. * * Cancellation marks this timer complete and canceled, but not failed, and - * notifies its composite parent. Reading `result` or `getResult()` then throws + * notifies its composite parents. Reading `result` or `getResult()` then throws * {@link TaskCancelledError}. A whenAny parent completes with this timer; * whenAll propagates cancellation when collecting its final child results. * * @returns true if cancellation was applied; false if already terminal. - * @throws If the cancel handler or parent completion callback throws. + * @throws If the cancel handler or a parent completion callback throws. */ cancel(): boolean { if (this._isComplete) { diff --git a/packages/durabletask-js/src/task/when-all-task.ts b/packages/durabletask-js/src/task/when-all-task.ts index 4d5a942..c24ee08 100644 --- a/packages/durabletask-js/src/task/when-all-task.ts +++ b/packages/durabletask-js/src/task/when-all-task.ts @@ -37,13 +37,15 @@ export class WhenAllTask extends CompositeTask { return result; } - onChildCompleted(_task: Task): void { - if (this._isComplete) { + onChildCompleted(task: Task): void { + const occurrences = this._pendingChildren.get(task); + if (this._isComplete || occurrences === undefined) { // Already completed (all children done). Ignore subsequent child completions. return; } - this._completedTasks++; + this._pendingChildren.delete(task); + this._completedTasks += occurrences; // Wait-all: a failing child does not complete the whenAll early; we wait until every child // is terminal. This prevents a later failing sibling's TaskFailed from being dropped against @@ -70,7 +72,7 @@ export class WhenAllTask extends CompositeTask { this._result = this._tasks.map((child) => child.getResult()); } - this._parent?.onChildCompleted(this); + this.notifyParents(); } } diff --git a/packages/durabletask-js/src/task/when-any-task.ts b/packages/durabletask-js/src/task/when-any-task.ts index 05ea5be..54b2ae2 100644 --- a/packages/durabletask-js/src/task/when-any-task.ts +++ b/packages/durabletask-js/src/task/when-any-task.ts @@ -19,7 +19,7 @@ export class WhenAnyTask extends CompositeTask> { if (!this.isComplete) { this._isComplete = true; this._result = task; - this._parent?.onChildCompleted(this); + this.notifyParents(); } } } diff --git a/packages/durabletask-js/test/composite-task.spec.ts b/packages/durabletask-js/test/composite-task.spec.ts new file mode 100644 index 0000000..be6dcdb --- /dev/null +++ b/packages/durabletask-js/test/composite-task.spec.ts @@ -0,0 +1,215 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { whenAll, whenAny } from "../src/task"; +import { CompletableTask } from "../src/task/completable-task"; +import { TimerTask } from "../src/task/timer-task"; +import { TaskCancelledError } from "../src/task/exception/task-cancelled-error"; +import { RetryableTask } from "../src/task/retryable-task"; +import { RetryHandlerTask } from "../src/task/retry-handler-task"; +import { RetryPolicy } from "../src/task/retry/retry-policy"; +import { RuntimeOrchestrationContext } from "../src/worker/runtime-orchestration-context"; +import { OrchestratorAction } from "../src/proto/orchestrator_service_pb"; + +describe("Composite task dependencies", () => { + it.each([ + { order: [0, 1, 2] }, + { order: [0, 2, 1] }, + { order: [1, 0, 2] }, + { order: [1, 2, 0] }, + { order: [2, 0, 1] }, + { order: [2, 1, 0] }, + ])("notifies both shared groups in completion order $order", ({ order }) => { + const tasks = ["A", "B", "C"].map(() => new CompletableTask()); + const first = whenAll([tasks[0], tasks[1]]); + const second = whenAll([tasks[0], tasks[2]]); + const root = whenAll([first, second]); + + for (const index of order) tasks[index].complete(["A", "B", "C"][index]); + + expect(first.isComplete).toBe(true); + expect(second.isComplete).toBe(true); + expect(root.getResult()).toEqual([ + ["A", "B"], + ["A", "C"], + ]); + }); + + it.each([false, true])( + "preserves duplicate positions with a mixed pending group (precompleted=%s)", + (precompleted) => { + const a = new CompletableTask(); + const b = new CompletableTask(); + if (precompleted) a.complete("A"); + const group = whenAll([a, a, b, a]); + + expect(group.completedTasks).toBe(precompleted ? 3 : 0); + b.complete("B"); + if (!precompleted) a.complete("A"); + + expect(group.completedTasks).toBe(4); + expect(group.pendingTasks()).toBe(0); + expect(group.getResult()).toEqual(["A", "A", "B", "A"]); + }, + ); + + it("preserves duplicates when every child is already complete", () => { + const task = new CompletableTask(); + task.complete("V"); + expect(whenAll([task, task]).getResult()).toEqual(["V", "V"]); + }); + + it("shares a pending composite between nested groups", () => { + const a = new CompletableTask(); + const b = new CompletableTask(); + const shared = whenAll([a, b]); + const left = whenAll([shared, shared]); + const right = whenAll([shared]); + const root = whenAll([left, right]); + + b.complete("B"); + a.complete("A"); + + expect(root.getResult()).toEqual([ + [ + ["A", "B"], + ["A", "B"], + ], + [["A", "B"]], + ]); + }); + + it.each(["fail", "failWithError"] as const)( + "notifies every dependent on %s without failing whenAll early", + (method) => { + const shared = new CompletableTask(); + const sibling = new CompletableTask(); + const first = whenAll([shared, sibling, shared]); + const second = whenAll([shared]); + const race = whenAny([shared]); + if (method === "fail") shared.fail("shared failure"); + else shared.failWithError(new Error("shared failure")); + const sharedError = shared.getException(); + + expect(first.isComplete).toBe(false); + expect(first.isFailed).toBe(false); + expect(first.pendingTasks()).toBe(1); + expect(second.getException()).toMatchObject({ errors: [sharedError] }); + expect(race.getResult()).toBe(shared); + + const siblingError = new Error("sibling failure"); + sibling.failWithError(siblingError); + expect(first.getException()).toMatchObject({ errors: [sharedError, siblingError, sharedError] }); + }, + ); + + it("shares whenAny's winner with multiple composite parents", () => { + const a = new CompletableTask(); + const b = new CompletableTask(); + const race = whenAny([a, b, a]); + const first = whenAll([race]); + const second = whenAll([race, race]); + const root = whenAll([first, second]); + + a.complete("A"); + expect(root.getResult()).toEqual([[a], [a, a]]); + b.complete("B"); + expect(race.getResult()).toBe(a); + }); + + it("lets whenAll and whenAny independently observe the same child", () => { + const a = new CompletableTask(); + const b = new CompletableTask(); + const all = whenAll([a, b]); + const race = whenAny([a, b]); + + a.complete("A"); + expect(race.getResult()).toBe(a); + expect(all.isComplete).toBe(false); + b.complete("B"); + expect(all.getResult()).toEqual(["A", "B"]); + }); + + it.each([false, true])("detaches whenAny from its losing children (precompleted=%s)", (precompleted) => { + const loser = new CompletableTask(); + const winner = new CompletableTask(); + if (precompleted) winner.complete("winner"); + const race = whenAny([loser, winner]); + if (!precompleted) winner.complete("winner"); + const completed = jest.spyOn(race, "onChildCompleted"); + + loser.complete("loser"); + + expect(completed).not.toHaveBeenCalled(); + expect(race.getResult()).toBe(winner); + }); + + it.each([false, true])("still notifies another observer when a canceled result throws (allFirst=%s)", (allFirst) => { + const timer = new TimerTask(); + const all = allFirst ? whenAll([timer]) : undefined; + const race = whenAny([timer]); + const throwingGroup = all ?? whenAll([timer]); + const parent = whenAll([throwingGroup]); + + expect(() => timer.cancel()).toThrow(TaskCancelledError); + + expect(race.getResult()).toBe(timer); + expect(throwingGroup.isComplete).toBe(true); + expect(() => throwingGroup.getResult()).toThrow("whenAll completed without a result"); + expect(parent.isComplete).toBe(false); + expect(timer.cancel()).toBe(false); + }); + + it("propagates all callback errors after notifying every observer", () => { + const timer = new TimerTask(); + const first = whenAll([timer]); + const second = whenAll([timer]); + const race = whenAny([timer]); + let failure: unknown; + try { + timer.cancel(); + } catch (error) { + failure = error; + } + + expect(failure).toBeInstanceOf(AggregateError); + expect(failure).toMatchObject({ errors: [expect.any(TaskCancelledError), expect.any(TaskCancelledError)] }); + expect(first.isComplete).toBe(true); + expect(second.isComplete).toBe(true); + expect(race.getResult()).toBe(timer); + }); + + it.each(["policy", "handler"] as const)("notifies shared %s retry dependents on success and exhaustion", (kind) => { + const makeTask = () => + kind === "policy" + ? new RetryableTask( + new RetryPolicy({ maxNumberOfAttempts: 2, firstRetryIntervalInMilliseconds: 1 }), + new OrchestratorAction(), + new Date(0), + "activity", + ) + : new RetryHandlerTask( + async () => false, + new RuntimeOrchestrationContext("retry"), + new OrchestratorAction(), + new Date(0), + "activity", + ); + const success = makeTask(); + const first = whenAll([success]); + const second = whenAll([success, success]); + success.recordFailure("retrying"); + success.complete("V"); + expect(first.getResult()).toEqual(["V"]); + expect(second.getResult()).toEqual(["V", "V"]); + expect(success.lastFailure).toBeUndefined(); + + const exhausted = makeTask(); + const failedFirst = whenAll([exhausted]); + const failedSecond = whenAll([exhausted, exhausted]); + exhausted.fail("exhausted"); + const error = exhausted.getException(); + expect(failedFirst.getException()).toMatchObject({ errors: [error] }); + expect(failedSecond.getException()).toMatchObject({ errors: [error, error] }); + }); +}); diff --git a/packages/durabletask-js/test/long-timer-replay-regressions.spec.ts b/packages/durabletask-js/test/long-timer-replay-regressions.spec.ts index 4f83b61..a44dff6 100644 --- a/packages/durabletask-js/test/long-timer-replay-regressions.spec.ts +++ b/packages/durabletask-js/test/long-timer-replay-regressions.spec.ts @@ -280,18 +280,20 @@ describe("segmented timer cancellation ordering", () => { expectCompleted(await replay(worker, history, [ph.newTaskCompletedEvent(2, '"done"')]), "done"); }); - it("cancels a shared timer winner without changing either race's task identity", () => { + it("preserves direct-child winner identity when a timer is shared between nested races", () => { const timer = new RuntimeOrchestrationContext("shared-timer").createTimer(atDay(10)); const work = new CompletableTask(); const firstRace = whenAny([timer, work]); const secondRace = whenAny([firstRace, timer]); timer.cancel(); + expect(firstRace.getResult()).toBe(timer); expect(secondRace.isComplete).toBe(true); - expect(secondRace.getResult()).toBe(timer); - expect(() => secondRace.getResult().getResult()).toThrow(TaskCancelledError); + // The first race observes the timer, then completes as the second race's direct child. + expect(secondRace.getResult()).toBe(firstRace); + expect(() => firstRace.getResult().getResult()).toThrow(TaskCancelledError); work.complete("done"); - expect(firstRace.getResult()).toBe(work); - expect(secondRace.getResult()).toBe(timer); + expect(firstRace.getResult()).toBe(timer); + expect(secondRace.getResult()).toBe(firstRace); }); it("leaves a caught whenAll completion exception as a boundary, not a reusable parent result", () => { diff --git a/packages/durabletask-js/test/task-composition.spec.ts b/packages/durabletask-js/test/task-composition.spec.ts new file mode 100644 index 0000000..1910ef7 --- /dev/null +++ b/packages/durabletask-js/test/task-composition.spec.ts @@ -0,0 +1,111 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { + InMemoryOrchestrationBackend, + TestOrchestrationClient, + TestOrchestrationWorker, + OrchestrationStatus, + whenAll, +} from "../src"; + +describe("Public task composition", () => { + let backend: InMemoryOrchestrationBackend; + let client: TestOrchestrationClient; + let worker: TestOrchestrationWorker; + let invocations: Map; + + beforeEach(async () => { + backend = new InMemoryOrchestrationBackend(); + client = new TestOrchestrationClient(backend); + worker = new TestOrchestrationWorker(backend); + invocations = new Map(); + worker.addNamedActivity("Echo", (context, value: string) => { + const values = invocations.get(context.orchestrationId) ?? []; + values.push(value); + invocations.set(context.orchestrationId, values); + return value; + }); + worker.addNamedOrchestrator("Shared", async function* (context): any { + const a = context.callActivity("Echo", "A"); + const b = context.callActivity("Echo", "B"); + const c = context.callActivity("Echo", "C"); + const first = whenAll([a, b]); + const second = whenAll([a, c]); + return yield whenAll([first, second]); + }); + worker.addNamedOrchestrator("Duplicate", async function* (context): any { + const task = context.callActivity("Echo", "V"); + return yield whenAll([task, task]); + }); + worker.addNamedOrchestrator("Disjoint", async function* (context): any { + const first = whenAll([context.callActivity("Echo", "A"), context.callActivity("Echo", "B")]); + const second = whenAll([context.callActivity("Echo", "C"), context.callActivity("Echo", "D")]); + return yield whenAll([first, second]); + }); + await worker.start(); + }); + + afterEach(async () => { + await worker.stop(); + await client.stop(); + backend.reset(); + }); + + it.each([ + { + name: "Shared", + values: ["A", "B", "C"], + output: [ + ["A", "B"], + ["A", "C"], + ], + }, + { name: "Duplicate", values: ["V"], output: ["V", "V"] }, + ])("completes $name without executing an activity twice on a reused worker", async ({ name, values, output }) => { + const controlId = await client.scheduleNewOrchestration("Disjoint"); + const control = await client.waitForOrchestrationCompletion(controlId, true, 3); + expect(control?.runtimeStatus).toBe(OrchestrationStatus.COMPLETED); + expect(control?.serializedOutput).toBe( + JSON.stringify([ + ["A", "B"], + ["C", "D"], + ]), + ); + expect(invocations.get(controlId)).toEqual(["A", "B", "C", "D"]); + + const id = await client.scheduleNewOrchestration(name); + const instance = await backend.waitForState( + id, + (instance) => { + const scheduled = instance.history + .filter((event) => event.hasTaskscheduled()) + .map((event) => event.getEventid()); + const completed = instance.history + .filter((event) => event.hasTaskcompleted()) + .map((event) => event.getTaskcompleted()!.getTaskscheduledid()); + return ( + scheduled.length === values.length && + completed.length === values.length && + new Set(scheduled).size === values.length && + new Set(completed).size === values.length && + scheduled.every((taskId) => completed.includes(taskId)) && + instance.pendingEvents.length === 0 + ); + }, + 3000, + ); + expect(instance?.history.filter((event) => event.hasTaskfailed())).toHaveLength(0); + expect(invocations.get(id)).toEqual(values); + expect(backend.hasPendingWork()).toBe(false); + + const state = await client.getOrchestrationState(id, true); + expect(state?.runtimeStatus).toBe(OrchestrationStatus.COMPLETED); + expect(state?.failureDetails).toBeUndefined(); + expect(state?.serializedOutput).toBe(JSON.stringify(output)); + expect(await client.waitForOrchestrationCompletion(id, true, 3)).toMatchObject({ + runtimeStatus: OrchestrationStatus.COMPLETED, + serializedOutput: JSON.stringify(output), + }); + }); +}); diff --git a/packages/durabletask-js/test/when-all-task.spec.ts b/packages/durabletask-js/test/when-all-task.spec.ts index 2c48219..53d8a78 100644 --- a/packages/durabletask-js/test/when-all-task.spec.ts +++ b/packages/durabletask-js/test/when-all-task.spec.ts @@ -153,9 +153,8 @@ describe("WhenAllTask", () => { const child2 = new CompletableTask(); const task = new WhenAllTask([child1, child2]); - // Spy parent to detect premature or duplicate notifications. - const parent = { onChildCompleted: jest.fn() }; - task._parent = parent as any; + const parent = new WhenAllTask([task]); + jest.spyOn(parent, "onChildCompleted"); // First child fails: under wait-all the WhenAll must not complete or notify yet. child1.fail("first failure");