Skip to content

feat: payload limits validation visitor#1327

Open
jmaeagle99 wants to merge 6 commits into
mainfrom
payload-limits-visitor
Open

feat: payload limits validation visitor#1327
jmaeagle99 wants to merge 6 commits into
mainfrom
payload-limits-visitor

Conversation

@jmaeagle99

@jmaeagle99 jmaeagle99 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

What was changed

Add a SDK-internal, build-time generated payload/memo limit visitor:

  • Generated from proto service definitions (every temporal.api.* rpc input message)
  • Emits PayloadLimitsValidatable implementations to report each payload-bearing field size and classification to a PayloadLimitsSink implementation.
  • Consumes the PAYLOAD_LIMITS_TABLE in build.rs to map each payload-bearing message and field combination to a classification. The build will fail if there are any missing or stale entries. This allows us to classify new combinations as they are added to the API. Initial classification answers built from inspection of temporal/temporal repository.
  • Algorithm terminates at Payload, Payloads, Memo, Failure, Any, SearchAttributes, and Header leaf fields.
  • APIs:
    • PayloadLimitSink receives callbacks from visitor to consume the size and classification information for each visited message and field combination.
    • validation_payload_limits func to log errors and warnings and returns the first error-level PayloadLimitViolation. Each violation carries a proto-field-name path that is relative to the root message.

Approximately 370 lines of build.rs is the classification table itself.
Approximately 300 lines of payload_limits.rs is tests.

Why?

Create a foundational element that will be later integrated to allow clients and workers to validate payload limits and proactively issue WFT / AT failures.

Checklist

  1. How was this tested: New unit tests
  2. Any docs updates needed? No

@jmaeagle99 jmaeagle99 force-pushed the payload-limits-visitor branch from 438ac14 to d2ddd07 Compare June 12, 2026 15:55
@jmaeagle99 jmaeagle99 force-pushed the payload-limits-visitor branch from d2ddd07 to 55c894c Compare June 12, 2026 16:25
@jmaeagle99 jmaeagle99 marked this pull request as ready for review June 12, 2026 16:26
@jmaeagle99 jmaeagle99 requested a review from a team as a code owner June 12, 2026 16:26
Comment thread crates/common/src/payload_limits.rs Outdated
Comment thread crates/common/build.rs Outdated
Comment thread crates/common/build.rs Outdated
Comment thread crates/common/src/payload_limits.rs
Comment thread crates/common/build.rs
}

#[derive(Clone, Copy)]
enum LeafKind {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like this is serving the same purpose as PayloadFieldKind which exists in the file already. Can we re-use that? Maybe it needs some extra variants. Broadly it seems like maybe some code could get shared here in general, AFAICT we don't re-use any of the existing visitor code but in principle I feel like we should've been able to extend that code to serve this purpose?

If it doesn't make sense, no prob, but, worth seeing if we can reduce LoC here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to share that specifically but both visitors have different terminal conditions and additional semantics carried with each of these enums, so I didn't refactor that. Was able to refactor and share more of the common message traversal and classification logic though. Ended with marginally less LoC but that might be due to comment add/removal.

@Sushisource Sushisource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, glad that zip suggestion reduced things a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants