Preserve legacy system payload visitor behavior - #309
Draft
stuart-wells wants to merge 1 commit into
Draft
Conversation
|
|
stuart-wells
marked this pull request as draft
September 1, 2026 21:49
Contributor
|
This fix looks right to me. I was unaware there was anything currently adding the metadata kvp to flag something as a system payload. Sorry about that! cc @tconley1428 who has more context on this, and can keep me honest. I think we need to be clear about only marking something as a "system payload" if it meets three criteria:
This will remove the failure for things that meet criteria (1) but not (2) and (3). But that should be considered a bug that we'd want to fix upstream, right? And only use the "system payload" for actual protobuf-encoded messages? |
5 tasks
chrsmith
added a commit
to temporalio/temporal
that referenced
this pull request
Sep 3, 2026
>⚠️ I'm a little out of my depth when it comes to the System Nexus Endpoint, and the current requirements around "system payloads". So please review this carefully to keep me honest. > > I'm not sure if this is the right way we want to address this problem, of if instead we should just relax the check in the SDK. (@stuart-wells opened temporalio/api-go#309, which we may also want to take. Independently of this.) ## Context The Temporal server was labeling _everything_ that came through the History Service's `StartNexusOperation` as a system payload. (#10948) However, when we landed support for visiting nested payloads in the Golang SDK (temporalio/api-go#297) it asserts that all system payloads have BOTH a `"encoding":"binary/protobuf"` AND `"messageType"` metadata key. That's a problem. Now, if the Temporal server were to pick up the latest `api-go` bits, it will introduce test failures. Because we have tests that return payloads encoded with `plain/json` that will fail when ran through the SDK's payload visitor at runtime. ## What changed? ~~This PR makes the requirements surrounding a system payload clearer, and ONLY flags a System Nexus Endpoint payload as a "system payload" IFF if is a properly labeled protobuf message. Otherwise, it doesn't set the system payload tag at all.~~ This PR now adds a check that the payload sent to the System Nexus Endpoint is a protobuf with message type available. It also updates a testcase that was sending `plain/json` responses to the SNE and added a new testcase. ## How did you test it? - [x] built - [x] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s) ## Potential risks We've already shipped code that includes the "label non-Protobuf payloads as system payloads". So it's possible that we've persisted those somewhere, and updating the `api-go` dependency (unless patched there) would cause problems.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Treat legacy marked, non-protobuf system payloads as ordinary payloads. Typed protobuf system payloads retain the validation introduced in #297.
Why?
Older Temporal servers marked all system Nexus result payloads with
__temporal_system_payload, including ordinary payloads without protobuf metadata. After #297, consuming these legacy payloads can fail because the visitor attempts to interpret them as typed protobuf envelopes.Newer servers will only mark typed protobuf payloads, but api-go still needs to handle responses and persisted or replicated history produced by older servers.
How did you test it?
Added a regression test covering a legacy marked
json/plainpayload and verified the existing malformed-protobuf tests still pass.Ran
make test.Potential risks
A marked payload without a message type and without protobuf encoding is now passed to the ordinary payload visitor instead of returning an error. Marked payloads that claim protobuf encoding, or include a message type with an incompatible encoding, continue to be validated strictly.