WCP 7/X: Change failure cause and expose workflow task completion size limit - #11377
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9e8dd172f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MemoSizeLimitError: int64(d.config.MemoSizeLimitError(info.Name)), | ||
| BlobSizeLimitError: int64(d.config.BlobSizeLimitError(info.Name)), | ||
| MemoSizeLimitError: int64(d.config.MemoSizeLimitError(info.Name)), | ||
| WorkflowTaskCompletionSizeLimitError: int64(d.config.WorkflowTaskCompletionBufferSizeLimit(info.Name)), |
There was a problem hiding this comment.
When pagination is enabled and the final page carries commands, this reports the buffer limit as the maximum total workflow-task-completion size, but AppendTaskCompletionPage enforces it only against intermediate-page commands (service/history/workflow/context.go:238-244); GetMergedTaskCompletionPages adds the final-page size only to a metric (context.go:346-350), and non-paginated completions bypass the limit entirely. Consequently, a server configured with a small limit can accept completions much larger than the value advertised to SDKs, so the new field cannot reliably guide their paging decisions; either enforce this value against the complete logical request or expose a limit whose documented semantics match the buffer-only check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'll think about this. But out of scope for this change.
| // Per-workflow completion buffer overflowed: terminate the workflow | ||
| wtFailedCause = newWorkflowTaskFailedCause( | ||
| enumspb.WORKFLOW_TASK_FAILED_CAUSE_PAYLOADS_TOO_LARGE, | ||
| enumspb.WORKFLOW_TASK_FAILED_CAUSE_REQUEST_TOO_LARGE, |
There was a problem hiding this comment.
just confirm my understanding that SDK will consume the new enum compatibly
There was a problem hiding this comment.
Discussed it offline. Not sure what you mean by "compatibly" here, can talk more if something is unclear.
…e limit (#11377) ## What changed? - Workflow task completion buffer overflow now fails the WFT with `WORKFLOW_TASK_FAILED_CAUSE_REQUEST_TOO_LARGE` instead of `PAYLOADS_TOO_LARGE`. - `DescribeNamespace` reports `NamespaceInfo.Limits.workflow_task_completion_size_limit_error`, sourced from `history.workflowTaskCompletionBufferSizeLimit`. API PR: temporalio/api#838 ## Why? `PAYLOADS_TOO_LARGE` is misleading here, no single payload is oversized, the request total is. Exposing the limit lets SDKs page under it instead of discovering it by failing. ## How did you test it? - [x] built - [ ] run locally and tested manually - [x] covered by existing tests - [x] added new unit test(s) - [x] added new functional test(s) ## Potential risks No
2 participants
What changed?
WORKFLOW_TASK_FAILED_CAUSE_REQUEST_TOO_LARGEinstead ofPAYLOADS_TOO_LARGE.DescribeNamespacereportsNamespaceInfo.Limits.workflow_task_completion_size_limit_error,sourced from
history.workflowTaskCompletionBufferSizeLimit.API PR: temporalio/api#838
Why?
PAYLOADS_TOO_LARGEis misleading here, no single payload is oversized, the request total is. Exposing the limit lets SDKs page under it instead of discovering it by failing.How did you test it?
Potential risks
No