fix: surface WorkflowAgent model stream error values without durable-step retries or duplicate callbacks - #18827
Conversation
Co-authored-by: MintedKenny <104696466+MintedKenny@users.noreply.github.com>
…145943955344 # Conflicts: # packages/harness-pi/src/pi-auth.test.ts
Bugfix reviewOutcome: changes-required Reproduction replayStatus: no-longer-reproduces The exact original reproduction completed successfully and the original bug signal did not appear. Fixes issueStatus: partially-addresses The fix works when doStreamStep executes as an ordinary function, but not reliably across its intended durable workflow step boundary. Concerns:
Side effectsRisk: high A terminal model error can now retry the entire model step and can invoke the public onError callback twice. Concerns:
PerformanceRisk: high Automatic step retries can multiply model-call latency, provider usage, streaming work, and cost on every terminal error part. Concerns:
Backwards compatibilityRisk: none No persisted-data schema or migration changes are introduced, and previously stored workflow data is unaffected. Breaking changesRisk: medium WorkflowAgent.stream now rejects on an error part where it previously resolved with an error finish reason, which is an externally observable error-behavior change. Concerns:
ArchitectureRisk: high Package boundaries remain intact, but using a thrown arbitrary value to cross a durable step boundary conflicts with the workflow runtime's retry and error-normalization architecture. Concerns:
Change scopeStatus: minimal The implementation, regression test, falsy-value tracking, and patch changeset are all directly related to the reported bug. SecurityRisk: low No code-execution, dependency, URL-handling, or package-boundary security issue is introduced, though thrown error messages can enter workflow failure records. TestingStatus: needs-more Node and edge unit tests cover direct Error and falsy rejection, but they bypass the workflow runtime behavior that makes the implementation unsafe. Concerns:
VerificationInspected the complete four-file diff and relevant workflow runtime implementation. The new node and edge regression tests passed, 130 related node tests passed, package type checking passed, and a direct probe confirmed falsy rejection but found duplicate onError calls. The repository workflow integration suite could not execute because of an unrelated workflow-runtime setup failure. Official workflow documentation and runtime code confirm that thrown step errors retry by default and are normalized before reaching workflow context. Relevant Documentation |
Co-authored-by: MintedKenny <104696466+MintedKenny@users.noreply.github.com> Co-authored-by: ai-sdk-factory[bot] <305873210+ai-sdk-factory[bot]@users.noreply.github.com> Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>
|
Addressed PR comment 5285148791.
Changed the durable-step contract so doStreamStep forwards the error part and returns its value as terminal data instead of throwing inside |
Bugfix reviewOutcome: changes-required Reproduction replayStatus: no-longer-reproduces The exact original reproduction completed successfully and the original bug signal did not appear. Fixes issueStatus: fully-addresses The error part is forwarded, returned across the durable step boundary as terminal data, and then thrown in workflow context; falsy values are retained and onError is invoked once. Side effectsRisk: medium The change deliberately converts a previously resolved error-finish path into a rejected WorkflowAgent.stream call. Concerns:
PerformanceRisk: none Returning terminal data avoids durable-step retries and stops consuming the stream after the terminal error, with no unbounded allocation or repeated model request. Backwards compatibilityRisk: none Existing stored workflow data is not rewritten or migrated; new runs persist the terminal value as an internal step-result variant. Breaking changesRisk: medium WorkflowAgent.stream now rejects for a standard model error part where the published behavior previously resolved with an error finish reason. Concerns:
ArchitectureRisk: none The terminal value crosses the retryable step as persisted data and is thrown only in workflow context, matching durable workflow boundaries and preserving package dependency direction. Change scopeStatus: minimal The implementation, callback adjustment, durable mock support, regression tests, integration workflow, and patch changeset are directly related to fixing and verifying the reported behavior. SecurityRisk: low No dependency, code-execution, URL-handling, or package-boundary risk was introduced; the supplied error value is now additionally stored as durable step data. TestingStatus: appropriate Tests cover direct Error identity, falsy terminal values, forwarding, exactly one callback, and a real durable workflow proving structured classification and one streamed error without retries. VerificationInspected the complete merge-base diff, public exports and result types, workflow serialization and retry behavior, and package boundaries. The workflow package's node and edge suites each passed 177 tests, the durable integration suite passed 20 tests including the new regression, package type checking passed, and focused serialization checks confirmed Error, structured-object, falsy, and undefined terminal values cross the step boundary. Relevant Documentation |
|
Addressed the remaining backwards-compatibility concern in 4f59cea. WorkflowAgent no longer rejects for a standard model stream error part; it keeps the existing resolved Verification: 178 Node tests, 178 Edge tests, 20 durable integration tests, full repository type-check, and repository lint/format all pass. |
gr2m
left a comment
There was a problem hiding this comment.
Reviewed the backward-compatible result contract, durable error transport, callback behavior, regression coverage, and full CI results. All concerns are addressed.
|
🚀 Published in:
|
Background
WorkflowAgent forwarded standard model stream error parts but discarded their original values from the server-side result.
Summary
result.error, including falsy andundefinedvalues (use'error' in resultto test presence).onErrorexactly once and document the new result field.Testing
pnpm type-check:fullpassed.pnpm checkpassed.Fixes #18824