[defect] recordChildResolution puts failure keys in ChildRunCompleted, so a completed child is reported to its parent as failed

[bilyiv]

Owning public component

Workflow engine

Exact version or source identity

2.0.7

Minimal reproduction and evidence

Reproduction

A child workflow whose body handles a failed activity and then finishes normally:

class ChildWorkflow extends Workflow
{
    public function handle(): string
    {
        try {
            activity(FailingActivity::class);   // exhausts its attempts, recording a WorkflowFailure
        } catch (Throwable) {
            // handled in the workflow body; the run carries on
        }

        return 'done';   // the child completes normally, with a failure recorded against it
    }
}

Call it from a parent with child(ChildWorkflow::class). The child reaches WorkflowCompleted while a WorkflowFailure row exists for it, and the parent then records the resolution.

This is a normal shape for any workflow with a failure-handling branch: a step fails, the workflow routes around it, the run ends successfully with a real result.

Verified on 2.0.7. The child's activity exhausted its single attempt, the body handled it, the walk continued and finished, and the child's own history then read:

ActivityScheduled → ActivityStarted → ActivityFailed → FailureHandled → ActivityCompleted
WorkflowCompleted   seq=13   06:38:48.513838   output = the child's real outcome
WorkflowFailed      seq=14   06:38:48.520169   ← 6.3ms later

The parent recorded ChildRunFailed and failed with it, having never seen the output.

Analysis

ActivityOutcomeRecorder writes a WorkflowFailure against the run when an activity stops retrying — not per attempt, and regardless of whether the workflow body then handles it (src/V2/Support/ActivityOutcomeRecorder.php:303-314, 'handled' => false). So a completed run can, and routinely does, own a failure row.

DefaultWorkflowTaskBridge::recordChildResolution() then builds one payload for every resolution type (src/V2/Support/DefaultWorkflowTaskBridge.php:4686-4724):

'failure_id' => $failure?->id,                       // ← never gated
'failure_category' => match ($eventType) {
    HistoryEventType::ChildRunFailed => …,
    HistoryEventType::ChildRunCancelled => …,
    HistoryEventType::ChildRunTerminated => …,
    default => null,                                 // ← gated, correctly
},
…
'exception_class' => $childTerminalEvent?->event_type === HistoryEventType::WorkflowFailed
    ? $childTerminalEvent->payload['exception_class'] ?? $failure?->exception_class
    : $failure?->exception_class,                    // ← non-failure branch, still set
'message' => $childTerminalEvent?->event_type === HistoryEventType::WorkflowFailed
    ? $childTerminalEvent->payload['message'] ?? $failure?->message
    : $failure?->message,                            // ← same

$failure is $childRun->failures->first() (line 4675) — any failure on the run, not one scoped to how it ended, and not filtered on handled.

That last part is the crux, because the engine already knows the failure was handled. When the workflow body handles a failure the engine records a FailureHandled history event and sets handled = true on the failure row (src/V2/Support/WorkflowExecutor.php:3983-3992). In the run reproduced above, the child's own rows read:

source_kind: activity_execution   exception_class: RuntimeException   handled: 1

with one FailureHandled event in its history. recordChildResolution() then picks that same row up and describes the child by it. The signal that distinguishes "a failure this run recovered from" from "the failure that ended this run" is recorded, and not consulted.

failure_category is gated on the event type; failure_id is not gated at all, and exception_class / message fall back to the failure row in the non-failure branch. The surrounding array_filter only drops nulls, so all three survive into a ChildRunCompleted.

The contract for that event type does not allow them (src/V2/Support/HistoryEventPayloadContract.php:544-565) — it lists only sequence, workflow_link_id, child_call_id, child_workflow_*, child_status, closed_reason, closed_at, output, result, payload_codec and the parallel_group_* keys.

WorkflowHistoryEvent::record() asserts the contract on every write (src/V2/Models/WorkflowHistoryEvent.php:82 → HistoryEventPayloadContract:901), so the write throws — and the child that already completed is then closed as failed.

Observed behavior

A child run that completed successfully is converted into a failed child, and its parent is told it failed.

Observed sequence, from the child's own history:

ActivityScheduled → ActivityStarted → ActivityFailed → FailureHandled
… (the workflow body continues and finishes normally) …
WorkflowCompleted        ← the child completed, with its real output recorded
WorkflowFailed           ← appended milliseconds later

The child ends status = failed, closed_reason = failed, carrying a second failure row:

source_kind: workflow_run
exception_class: InvalidArgumentException
message: ChildRunCompleted history payload contains undocumented key(s):
         failure_id, exception_class, message.

and the parent records ChildRunFailed for it, taking its failure path.

Three consequences:

1. The child's real result is discarded. The run completed and produced a valid outcome; the parent never sees it.
2. The parent branches on a failure that did not happen — a successful child is indistinguishable, to the caller, from one that genuinely failed.
3. The child's history holds two terminal events, WorkflowCompleted followed by WorkflowFailed, which no consumer of that history can reconcile.

Nothing in the calling application can influence which keys recordChildResolution() includes, and assertKnownPayloadKeys() is called unconditionally from WorkflowHistoryEvent::record() (src/V2/Models/WorkflowHistoryEvent.php:82) — there is no setting that relaxes it.

Reproduced on 2.0.7 from a two-document case, and seen before that across six unrelated runs — always the same three keys.

Expected behavior and acceptance criteria

Expected: a child that completes successfully resolves as completed on its parent, and its output reaches the caller — whether or not failures were recorded during its run. Failure-describing keys belong only on failure-shaped resolutions.

Acceptance criteria:

1. recordChildResolution() gates failure_id, exception_class and message on the resolution event type, as failure_category already is — and does not describe a child by a failure the engine has already marked handled.
2. A child that records a WorkflowFailure and then completes produces a valid ChildRunCompleted payload; the parent records ChildRunCompleted and receives the child's output.
3. A run that has recorded WorkflowCompleted is never subsequently closed as failed. A failure raised while recording a resolution is a fault in the parent's recording, not a retroactive verdict on a child that already finished — a run's history should not be able to hold two terminal events.
4. Regression test: child with a failed activity handled inside the workflow body → completes → parent resolves it as completed, without assertKnownPayloadKeys() throwing.
5. Either the contract or the writer is authoritative for every event type; today they disagree for ChildRunCompleted.

Dependencies and related public issues

None.

Public intake checks

• I searched open and closed GitHub issues for an existing report.
• This report contains only public product context and redacted evidence.

[rmcdaniel]

Accepted as P1. This is distinct from the missing wake-up in #493, but it affects the same child-resolution path. Source review confirms that both the embedded executor and service-mode bridge can attach a historical failure row to a successful ChildRunCompleted event, contrary to that event contract.

I am reproducing the handled-activity-failure case and checking the terminal-state boundary as well: successful children must retain their result, handled failures must not leak into success payloads, and a parent-notification fault must not retroactively append WorkflowFailed to a completed child. This will be addressed before releasing the child-completion work in #494. Cancellation, genuine failure, and recovery will be checked separately so the fix does not suppress legitimate errors.

[rmcdaniel]

Reproduced in both paths against the current source: the embedded child ends failed after handling the activity error, while the service bridge throws ChildRunCompleted history payload contains undocumented key(s): failure_id, exception_class, message.

The candidate fix now passes both reproductions. It selects failure metadata only for a non-success terminal outcome, ignores handled failures, and prefers the failure referenced by terminal history. An injected parent-notification error also proves the completion attempt rolls back and remains repairable instead of committing contradictory completed/failed events; retry then delivers the original child result. Focused outcome, mixed-group, cancellation and repair checks pass (26 tests, 305 assertions; one MySQL-only concurrency test skipped in this SQLite run). MySQL and full-suite qualification follow on #494.

[rmcdaniel]

The engine fix is merged in #494 and published as Workflow 2.0.8. Both embedded execution and the service bridge now keep historical activity failures out of successful child results. Genuine terminal failures retain their own failure reference, and a parent-notification infrastructure fault cannot append WorkflowFailed after WorkflowCompleted; the task transaction rolls back and remains repairable.

The embedded and service-mode reproductions, notification-fault recovery, and full source matrix passed. Clean published-package upgrade checks are running: https://github.com/durable-workflow/workflow/actions/runs/34233521331 .

Standalone Server pins the engine package, so it also needs a new image. The dependency-only Server 2.3.4 / Helm 0.1.83 update is being qualified separately. No existing contradictory terminal histories are automatically rewritten, and this does not claim a Cloud rollout.

[rmcdaniel]

Released and verified in Workflow 2.0.8, via #494. The embedded and service-bridge regressions, notification-fault recovery and genuine terminal failure tests pass. All published-package upgrade checks passed: https://github.com/durable-workflow/workflow/actions/runs/34233521331 .

Update the package and restart long-lived workers. No database migration is required. Existing contradictory terminal histories are not automatically rewritten.

Standalone Server delivery is separately tracked in durable-workflow/server#145, because its image pins the engine dependency. Closing this engine defect does not imply that Server or Cloud has already been deployed.