Repository navigation
Write concurrent oneof payload visits back to the message - #324
Merged
Merged
Conversation
The payload visitor special-cases payloads held directly in a oneof (DescribeNexusOperationExecutionResponse.result, PollNexusOperationExecutionResponse.result, OnCompleteRequest.success) by visiting a local copy and assigning a new oneof wrapper from it. With ConcurrencyLimit > 1, visitPayload returns before the visitor runs, so the wrapper is assigned from the original payload, the visitor's result is dropped, and its later write to the copy races with that assignment. Visit through the field of the wrapper already in the message instead, so a deferred visitor writes into the message as it does for plain payload fields.
|
|
| Visitor: func(ctx *VisitPayloadsContext, p []*common.Payload) ([]*common.Payload, error) { | ||
| // Let the generated code move on before the result exists, as a | ||
| // visitor doing real work would. | ||
| time.Sleep(10 * time.Millisecond) |
Contributor
There was a problem hiding this comment.
I think there's some time bubble thing you can use to avoid sleeps / nondetermitism? Might be worth a property test
cf / get claude to https://github.com/flyingmutant/rapid
Hold the visitor on a channel until the walk returns, instead of sleeping, so its result always arrives after the walk is done. The test then fails on the copy-and-assign code every run, without relying on timing or -race.
| {{if and (eq $type "*workflowservice.DescribeNexusOperationExecutionResponse") (eq . "Result")}} | ||
| if o.GetResult() != nil { | ||
| result := o.GetResult() | ||
| if err := visitPayload(ctx, options, o, concState, &result); err != nil { return err } |
Contributor
There was a problem hiding this comment.
I didn't realise this doesn't block and just does background work.
😡 this is ... not good as a pattern imho
davidporter-id-au
approved these changes
Nov 4, 2026
bergundy
approved these changes
Nov 4, 2026
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
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?
The payload visitor visits payloads held directly in a oneof through the field of the oneof wrapper already in the message, instead of through a local copy that is then assigned back. This covers
DescribeNexusOperationExecutionResponse.result,PollNexusOperationExecutionResponse.result, andOnCompleteRequest.success.Why?
With
ConcurrencyLimit > 1,visitPayloadreturns before the visitor runs. Assigning the oneof from the copy then keeps the original payload, drops the visitor's result, and races with the visitor's later write to the copy.How did you test it?
TestVisitPayloadsConcurrentOneofPayloadcovers all three fields. It fails onmain, with the original payload returned and a data race reported under-race, and passes with this change.make check testPotential risks
The existing oneof wrapper is updated in place instead of replaced, matching how plain payload fields are visited. Sequential visiting is otherwise unchanged.