💥 Payload limit enforcement#1363
Conversation
c42f207 to
aebcc45
Compare
aebcc45 to
a9f694c
Compare
jmaeagle99
left a comment
There was a problem hiding this comment.
Some self-review comments. If you have ideas/opinions, would love to hear them.
Sushisource
left a comment
There was a problem hiding this comment.
Making sense to me overall. Rubber ducked myself on the one comment here.
| // Caught client-side, so we fail the activity task | ||
| // ourselves (the server otherwise would have) and stop | ||
| // the activity with the same `Cancelled`/`cancel_requested` | ||
| // signal the server uses. | ||
| if let Err(fe) = sg | ||
| .fail_activity_task( | ||
| tt.clone(), | ||
| Some(make_payloads_too_large_failure(violation)), | ||
| ) | ||
| .await | ||
| { | ||
| warn!(task_token = %tt, error = ?fe, | ||
| "Failed to fail activity after oversized heartbeat"); | ||
| } |
There was a problem hiding this comment.
Hmm. I feel like it would be better if we could fail lang's heartbeat call rather than failing the activity attempt. Then the user can decide if they want to fail the activity or not.
But I acknowledge we're not set up super well to do that at the moment. In fact there's a very old TODO that would also be able to be fixed if we changed that: https://github.com/temporalio/sdk-core/blob/3c605ee7e4ba10f007f89de55b67862e0e9b9ba9/crates/sdk-core/src/worker/activities.rs#L445
It'd involve changing Worker::record_activity_heartbeat to be async and actually return an error that lang can react to, and then I suppose in order to make it work with the whole buffering concept we'd somehow need to decide if this particular call is going to be the one that actually sends the data, and if so, block and return the error if there was one.
That's a lot, obviously, and from what I can see wouldn't even be possible to do compatibly in some languages (ie: would require changes to their heartbeat calls). So I suppose after writing all that, we probably can't do it. Sorta sucks, but this is still better than what we had before of course so I suppose it'll just need to be this way.
What was changed
Builds on the existing payload-field classification codegen to add end-to-end payload/memo
size-limit enforcement in the client and worker.
PayloadLimitViolationas its source.ConnectionOptionsaspayload_limits PayloadLimitsOptionsfield. Default is to warn payloads at 512 KiB and memos at 2 KiB, same defaults as server.PayloadLimitsClientdecorator that wraps the existing connection decorator. Each request has the error limits (if set) attached as a request extension.RespondWorkflowTaskFailedRespondActivityTaskFailedRespondActivityTaskFailedand report activity as cancelledRespondNexusTaskFailedFailureReason::PayloadsTooLargemetric reason.💥 Breaking Change
Payload size-limit enforcement is now on by default; existing code changes behavior without opting in: an oversized payload is now failed proactively by the worker (as a retryable task failure) before it's sent, rather than being sent and hard-failed by the server as before.
If you use a proxy between the worker and server that alters the size of payloads (e.g.
compression, encryption, external storage), it is advised that you disable size enforcement by
setting
disable_payload_error_limittotrueon the worker.Why?
Prevent server from hard-failing workflows, activities, and operations due to payloads/memo size limit violations. Allows customers to potentially update their workflows, activities, and operations to reduce the oversized payloads/memos.
Checklist