Skip to content

[copilot-finds] Bug: Truthy check on taskId in completion event handlers silently skips taskId 0 #148

@github-actions

Description

@github-actions

Problem

In orchestration-executor.ts, the handleCompletedTask (line 697) and handleSubOrchestrationCompleted (line 396) methods use truthy checks (if (taskId)) to guard task lookups from ctx._pendingTasks. Since taskId is a number | undefined, this incorrectly treats taskId === 0 as "no taskId" because 0 is falsy in JavaScript.

This is inconsistent with handleFailedTask (line 724) and handleTimerFired (line 293) in the same file, which correctly use if (taskId === undefined).

Root Cause

JavaScript truthy evaluation treats 0 as falsy. When getTaskscheduledid() returns 0 (the proto3 default for unset int32 fields), the truthy check if (taskId) evaluates to false, skipping the task lookup entirely. The event is then silently dropped without the task being completed.

Proposed Fix

Change both truthy checks from if (taskId) to if (taskId !== undefined), matching the pattern used by handleFailedTask and handleTimerFired.

Impact

Severity: Low-Medium. The SDK currently uses 1-indexed sequence numbers (via nextSequenceNumber()), so taskId 0 does not occur in normal operation. However:

  • In proto3, an unset int32 field defaults to 0, so a malformed or partially-constructed event from the sidecar could trigger this.
  • The inconsistency with other event handlers in the same file creates a maintenance risk.
  • If the numbering scheme ever changes to 0-indexed (as used by other Durable Task SDKs), this would become a production bug causing orchestrations to hang.

Metadata

Metadata

Assignees

No one assigned

    Labels

    copilot-findsFindings from daily automated code review agent

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions