Skip to content

fix(workflow): prevent concurrent map writes on shared error singletons#2714

Open
nankingjing wants to merge 2 commits into
coze-dev:mainfrom
nankingjing:fix-workflow-concurrent-map-write
Open

fix(workflow): prevent concurrent map writes on shared error singletons#2714
nankingjing wants to merge 2 commits into
coze-dev:mainfrom
nankingjing:fix-workflow-concurrent-map-write

Conversation

@nankingjing

Copy link
Copy Markdown

What this PR does

Fixes a fatal error: concurrent map writes crash in workflow execution (reported in #2713).

Root cause

WorkflowError values carry an error level. The level was stored inside the
underlying errorx.StatusError's Extra() map, and ChangeErrLevel mutated
that map in place:

func (w *wfErr) ChangeErrLevel(newLevel ErrorLevel) WorkflowError {
    w.StatusError.Extra()["level"] = string(newLevel) // in-place write
    return w
}

Several WorkflowError values are package-level singletons that are shared by
every goroutine:

var CancelErr = newCancel()
var NodeTimeoutErr = newNodeTimeout()
var WorkflowTimeoutErr = newWorkflowTimeout()

When a node fails, (*nodeRunner).onError assigns one of these shared
singletons (e.g. sErr = vo.NodeTimeoutErr on context.DeadlineExceeded) and
then calls sErr.ChangeErrLevel(vo.LevelWarn). Under concurrent workflow
execution — e.g. many requests whose HTTP nodes time out at the same time —
multiple goroutines write to the same shared map simultaneously, which Go
detects as a fatal, unrecoverable concurrent map writes and crashes the
whole process. This exactly matches the stack trace in #2713
(node.go:151 in ChangeErrLevel, node_runner.go:848 in onError).

The fix

  • Give wfErr its own level ErrorLevel field instead of storing the level in
    the shared Extra() map.
  • ChangeErrLevel now returns a copy of the error with the new level and
    never mutates the receiver, so the shared singletons are never modified.
  • Level() prefers the new field and falls back to the previous Extra()
    lookup, so behavior is unchanged for existing callers (event_handle.go).
  • Constructors populate the field; the Extra("level", ...) metadata is kept
    for backward compatibility.

After the fix the singletons' Extra() map is only written once at
construction and is read-only afterwards, so concurrent access is safe.

Tests

Added node_concurrency_test.go:

  • TestChangeErrLevelIsSafeForConcurrency — 100 goroutines call
    ChangeErrLevel on the shared singletons concurrently. This reproduces the
    crash on the old code and passes on the new code (also clean under -race).
  • TestChangeErrLevelDoesNotMutateSingleton — verifies ChangeErrLevel
    returns a copy and leaves the singleton's level untouched.

Fixes #2713

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.

工作流错误处理时引发 fatal error: concurrent map writes

1 participant