Skip to content

Commit 71db02c

Browse files
committed
fix: guard sub-orchestration completion handler against missing pending tasks
Refactor handleSubOrchestrationCompleted to delegate to handleCompletedTask, matching the pattern used by handleSubOrchestrationFailed -> handleFailedTask and handleTaskCompleted -> handleCompletedTask. Before this fix, handleSubOrchestrationCompleted called ctx.resume() unconditionally, even when no matching pending task was found. This differs from handleCompletedTask (the activity completion handler), which correctly returns early with a warning log when no matching task exists. The unconditional resume() could advance the generator incorrectly if _previousTask happens to be complete from an unrelated event. Additionally, orphaned sub-orchestration completion events were silently dropped without any diagnostic logging, unlike the activity handler. This change: - Adds the missing guard clause (returns early when no task found) - Adds warning logging for unexpected events (via WorkerLogs) - Adds isEmpty normalization for empty results (consistent with activity handler) - Reduces code duplication by reusing handleCompletedTask Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 29455dd commit 71db02c

2 files changed

Lines changed: 32 additions & 19 deletions

File tree

packages/durabletask-js/src/worker/orchestration-executor.ts

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -386,25 +386,11 @@ export class OrchestrationExecutor {
386386
}
387387

388388
private async handleSubOrchestrationCompleted(ctx: RuntimeOrchestrationContext, event: pb.HistoryEvent): Promise<void> {
389-
const subOrchestrationInstanceCompletedEvent = event.getSuborchestrationinstancecompleted();
390-
const taskId = subOrchestrationInstanceCompletedEvent
391-
? subOrchestrationInstanceCompletedEvent.getTaskscheduledid()
392-
: undefined;
393-
394-
let subOrchTask;
395-
396-
if (taskId !== undefined) {
397-
subOrchTask = ctx._pendingTasks[taskId];
398-
delete ctx._pendingTasks[taskId];
399-
}
400-
401-
const result = parseJsonField(subOrchestrationInstanceCompletedEvent?.getResult());
402-
403-
if (subOrchTask) {
404-
subOrchTask.complete(result);
405-
}
406-
407-
await ctx.resume();
389+
const completedEvent = event.getSuborchestrationinstancecompleted();
390+
const taskId = completedEvent ? completedEvent.getTaskscheduledid() : undefined;
391+
const result = completedEvent?.getResult();
392+
const normalizedResult = isEmpty(result) ? undefined : result;
393+
await this.handleCompletedTask(ctx, taskId, normalizedResult, "subOrchestrationInstanceCompleted");
408394
}
409395

410396
private async handleSubOrchestrationFailed(ctx: RuntimeOrchestrationContext, event: pb.HistoryEvent): Promise<void> {

packages/durabletask-js/test/orchestration_executor.spec.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,33 @@ describe("Orchestration Executor", () => {
570570
// assert user_code_statement in complete_action.failureDetails.stackTrace.value
571571
});
572572

573+
it("should not advance the generator when a sub-orchestration completion event has no matching pending task", async () => {
574+
const subOrchestrator = async (_: OrchestrationContext) => {
575+
// do nothing
576+
};
577+
const orchestrator: TOrchestrator = async function* (ctx: OrchestrationContext, _: any): any {
578+
const res = yield ctx.callSubOrchestrator(subOrchestrator);
579+
return res;
580+
};
581+
const registry = new Registry();
582+
const subOrchestratorName = registry.addOrchestrator(subOrchestrator);
583+
const orchestratorName = registry.addOrchestrator(orchestrator);
584+
const oldEvents = [
585+
newOrchestratorStartedEvent(),
586+
newExecutionStartedEvent(orchestratorName, TEST_INSTANCE_ID, undefined),
587+
newSubOrchestrationCreatedEvent(1, subOrchestratorName, "sub-orch-123"),
588+
];
589+
// Send a completion event with a taskId (999) that does not match any pending task.
590+
// Before the fix, this would call resume() unconditionally. After the fix, it returns
591+
// early without advancing the generator, consistent with handleCompletedTask behavior.
592+
const newEvents = [newSubOrchestrationCompletedEvent(999, JSON.stringify("unexpected"))];
593+
const executor = new OrchestrationExecutor(registry, testLogger);
594+
const result = await executor.execute(TEST_INSTANCE_ID, oldEvents, newEvents);
595+
// The orchestration should still be waiting for the real sub-orchestration to complete.
596+
// No complete action should be produced.
597+
expect(result.actions.length).toEqual(0);
598+
});
599+
573600
it("should test that an orchestration can wait for and process an external event sent by a client", async () => {
574601
const orchestrator: TOrchestrator = async function* (ctx: OrchestrationContext, _: any): any {
575602
const res = yield ctx.waitForExternalEvent("my_event");

0 commit comments

Comments
 (0)