Skip to content

Commit

Permalink
Hopefully avoid panic in DescribeWorkflowExecution (#5057)
Browse files Browse the repository at this point in the history
**What changed?**

Clone suspected mutating fields after a returning from
DescribeWorkflowExecution.

**Why?**

Noticed panics in serialization of the
`DescribeWorkflowExecutionResponse` object.

**How did you test it?**

No tests added, assuming that normal test coverage is enough.
Haven't reproduced the issue, this is based on a hunch.

**Potential risks**

None that I can foresee.

**Is hotfix candidate?**

Yes.
  • Loading branch information
bergundy authored and rodrigozhou committed Nov 1, 2023
1 parent ddf9799 commit 26114d7
Showing 1 changed file with 46 additions and 10 deletions.
56 changes: 46 additions & 10 deletions service/history/api/describeworkflow/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,21 @@ import (
"go.temporal.io/server/service/history/workflow"
)

func clonePayloadMap(source map[string]*commonpb.Payload) map[string]*commonpb.Payload {
target := make(map[string]*commonpb.Payload, len(source))
for k, v := range source {
metadata := make(map[string][]byte, len(v.GetMetadata()))
for mk, mv := range v.GetMetadata() {
metadata[mk] = mv
}
target[k] = &commonpb.Payload{
Metadata: metadata,
Data: v.GetData(),
}
}
return target
}

func Invoke(
ctx context.Context,
req *historyservice.DescribeWorkflowExecutionRequest,
Expand Down Expand Up @@ -73,11 +88,29 @@ func Invoke(
if err != nil {
return nil, err
}
// We release the lock on this workflow just before we return from this method, at which point mutable state might
// be mutated. Take extra care to clone all response methods as marshalling happens after we return and it is unsafe
// to mutate proto fields during marshalling.
defer func() { weCtx.GetReleaseFn()(retError) }()

mutableState := weCtx.GetMutableState()
executionInfo := mutableState.GetExecutionInfo()
executionState := mutableState.GetExecutionState()

resetPoints := &workflowpb.ResetPoints{
Points: make([]*workflowpb.ResetPointInfo, len(executionInfo.AutoResetPoints.GetPoints())),
}
for i, p := range executionInfo.AutoResetPoints.GetPoints() {
resetPoints.Points[i] = &workflowpb.ResetPointInfo{
BinaryChecksum: p.BinaryChecksum,
RunId: p.RunId,
FirstWorkflowTaskCompletedId: p.FirstWorkflowTaskCompletedId,
CreateTime: p.CreateTime,
ExpireTime: p.ExpireTime,
Resettable: p.Resettable,
}
}

result := &historyservice.DescribeWorkflowExecutionResponse{
ExecutionConfig: &workflowpb.WorkflowExecutionConfig{
TaskQueue: &taskqueuepb.TaskQueue{
Expand All @@ -93,14 +126,13 @@ func Invoke(
WorkflowId: executionInfo.WorkflowId,
RunId: executionState.RunId,
},
Type: &commonpb.WorkflowType{Name: executionInfo.WorkflowTypeName},
StartTime: executionInfo.StartTime,
Status: executionState.Status,
HistoryLength: mutableState.GetNextEventID() - common.FirstEventID,
ExecutionTime: executionInfo.ExecutionTime,
Memo: &commonpb.Memo{Fields: executionInfo.Memo},
SearchAttributes: &commonpb.SearchAttributes{IndexedFields: executionInfo.SearchAttributes},
AutoResetPoints: executionInfo.AutoResetPoints,
Type: &commonpb.WorkflowType{Name: executionInfo.WorkflowTypeName},
StartTime: executionInfo.StartTime,
Status: executionState.Status,
HistoryLength: mutableState.GetNextEventID() - common.FirstEventID,
ExecutionTime: executionInfo.ExecutionTime,
// Memo and SearchAttributes are set below
AutoResetPoints: resetPoints,
TaskQueue: executionInfo.TaskQueue,
StateTransitionCount: executionInfo.StateTransitionCount,
HistorySizeBytes: executionInfo.GetExecutionStats().GetHistorySize(),
Expand Down Expand Up @@ -208,8 +240,12 @@ func Invoke(
)
return nil, serviceerror.NewInternal("Failed to fetch memo and search attributes")
}
result.WorkflowExecutionInfo.Memo = relocatableAttributes.Memo
result.WorkflowExecutionInfo.SearchAttributes = relocatableAttributes.SearchAttributes
result.WorkflowExecutionInfo.Memo = &commonpb.Memo{
Fields: clonePayloadMap(relocatableAttributes.Memo.GetFields()),
}
result.WorkflowExecutionInfo.SearchAttributes = &commonpb.SearchAttributes{
IndexedFields: clonePayloadMap(relocatableAttributes.SearchAttributes.GetIndexedFields()),
}

return result, nil
}

0 comments on commit 26114d7

Please sign in to comment.