Skip to content

fix(workflow): prevent nil pointer dereference when merging batch inner node executions#2715

Open
nankingjing wants to merge 2 commits into
coze-dev:mainfrom
nankingjing:fix-merge-composite-nil
Open

fix(workflow): prevent nil pointer dereference when merging batch inner node executions#2715
nankingjing wants to merge 2 commits into
coze-dev:mainfrom
nankingjing:fix-merge-composite-nil

Conversation

@nankingjing

Copy link
Copy Markdown

What type of PR is this?

fix

What this PR does / why we need it

mergeCompositeInnerNodes (in backend/domain/workflow/service/executable_impl.go)
builds its result by iterating the nodeExes map to seed a groupNodeExe
pointer, then unconditionally writes to it:

var groupNodeExe *entity.NodeExecution
for _, v := range nodeExes {
    groupNodeExe = &entity.NodeExecution{ ... }
    break
}
...
groupNodeExe.IndexedExecutions = make([]*entity.NodeExecution, maxIndex+1) // nil deref if nodeExes is empty

When nodeExes is empty, groupNodeExe stays nil and the subsequent write
panics with a nil pointer dereference.

This path is reachable from GetNodeExecution. When a node running in batch
mode
(e.g. an LLM node with batch enabled, which is compiled into a
NodeTypeBatch parent + a generated <id>_inner node) is node-debugged, the
function queries the inner executions via GetNodeExecutionByParent. If that
returns an empty slice — for example the batch iterated over an empty input
list, or the inner executions have not been persisted yet — the loop that
checks IsGeneratedNodeForBatchMode does not run, index2Exe is left empty,
and mergeCompositeInnerNodes(index2Exe, 0) is called with an empty map,
triggering the panic in the request goroutine.

The fix returns early (nil) when there are no inner executions to merge.
GetNodeExecution then returns (nodeExe, nil, nil), which is consistent with
the other early-return branches of the function (a nil inner execution is
already an expected value for callers such as GetLatestNodeDebugInput). The
other caller, GetExecution, only ever passes non-empty maps, so its behavior
is unchanged.

Which issue(s) this PR fixes

Related to #2684 (behavior of GetNodeExecution for batch / batch-mode nodes).

Test plan

Adds TestImpl_GetNodeExecution covering:

  • node execution not found -> error
  • non-batch node -> returns node execution directly
  • batch node not in node-debug mode -> returns node execution directly
  • batch node in node-debug mode with no inner executions -> no panic (regression)
  • batch-mode node with generated inner executions -> merged inner execution returned
  • normal batch node with non-generated inner nodes -> returns node execution directly

The tests follow the existing table-driven gomock style in
executable_impl_test.go.

Note: verified by reading types/signatures and tracing the code paths; the Go
build and tests were not executed in my environment.

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.

1 participant