From c53a49e540cc0929799bc6efee9f39e9b2a5746e Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Wed, 24 Jun 2026 20:11:14 +0200 Subject: [PATCH] controllogic: fix data race in setPath nested-array assignment setPath mutated nested sub-arrays in place; since graph-local slices (and their nested sub-slices) may be read concurrently by other flow goroutines, an action.array.set with a multi-level path raced readers of the same local. The single-threaded TS source mutates in place safely, but the Go port runs flows on concurrent goroutines, so setPath now copies on descent (every level returns a freshly allocated slice). Adds a -race regression test that concurrently drives nested set + inner-element reads, plus a nested-set correctness test asserting the prior stored value is not mutated. Found by whole-branch review (Opus); missed by the per-task reviews. Co-Authored-By: Claude Opus 4.6 --- internal/controllogic/engine.go | 21 +++++--- internal/controllogic/engine_test.go | 74 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/internal/controllogic/engine.go b/internal/controllogic/engine.go index 3f1bab8..a35cca5 100644 --- a/internal/controllogic/engine.go +++ b/internal/controllogic/engine.go @@ -277,7 +277,11 @@ func (e *Engine) liveGet(ds, name string) float64 { } // setPath assigns v at the nested index path within arr, growing (zero-filling) -// as needed and resolving negative indices against length. Port of logic.ts. +// as needed and resolving negative indices against length. Adapted from logic.ts; +// unlike the single-threaded TS source it copies on descent rather than mutating in +// place, because a graph-local slice (and its nested sub-slices) may be read +// concurrently by other flow goroutines — in-place mutation of shared backing would +// race. Every level returns a freshly allocated slice. func setPath(arr []Value, path []int, v Value) []Value { if len(path) == 0 { return arr @@ -289,16 +293,17 @@ func setPath(arr []Value, path []int, v Value) []Value { if k < 0 { return arr } - for len(arr) <= k { - arr = append(arr, 0.0) + out := append([]Value{}, arr...) // copy: backing may be shared with the live local + for len(out) <= k { + out = append(out, 0.0) } if len(path) == 1 { - arr[k] = v - return arr + out[k] = v + return out } - sub, _ := arr[k].([]Value) - arr[k] = setPath(sub, path[1:], v) - return arr + sub, _ := out[k].([]Value) + out[k] = setPath(sub, path[1:], v) + return out } // write applies an action.write/lua-set to a target: a bare name updates a diff --git a/internal/controllogic/engine_test.go b/internal/controllogic/engine_test.go index e092784..4dcc41d 100644 --- a/internal/controllogic/engine_test.go +++ b/internal/controllogic/engine_test.go @@ -3,6 +3,7 @@ package controllogic import ( "context" "reflect" + "sync" "testing" ) @@ -108,6 +109,79 @@ func TestArrayPopNode(t *testing.T) { } } +// TestArraySetNodeNested verifies set with a comma-separated path mutates a nested +// sub-array, and (crucially) that setPath does not mutate the previously stored slice +// in place — the stored value must be replaced by a fresh tree. +func TestArraySetNodeNested(t *testing.T) { + g := Graph{ + ID: "g", Name: "n", + StateVars: []StateVar{{Name: "grid", Type: "array", Initial: "[[1,2],[3,4]]", Sizing: "dynamic"}}, + Nodes: []Node{ + {ID: "t", Kind: "trigger.timer", Params: map[string]string{"interval": "1000"}}, + {ID: "s", Kind: "action.array.set", Params: map[string]string{"array": "grid", "index": "0,1", "expr": "9"}}, + }, + Wires: []Wire{{From: "t", To: "s"}}, + } + cg := compile(g) + before := cg.getLocal("grid") + cg.engine = &Engine{} + cg.genCtx = context.Background() + R := func(ds, name string) Value { + if ds == "local" { + return cg.getLocal(name) + } + return 0.0 + } + cg.follow("t", "out", &runCtx{fired: "t", resolve: R}) + want := []Value{[]Value{1.0, 9.0}, []Value{3.0, 4.0}} + if got := cg.getLocal("grid"); !reflect.DeepEqual(got, want) { + t.Fatalf("after nested set grid = %#v", got) + } + // The pre-mutation snapshot must be untouched (no shared-backing in-place write). + if !reflect.DeepEqual(before, []Value{[]Value{1.0, 2.0}, []Value{3.0, 4.0}}) { + t.Fatalf("setPath mutated the prior stored value in place: %#v", before) + } +} + +// TestArraySetNodeConcurrentNoRace drives many concurrent set+read flows against the +// same nested-array local; with -race it guards the setPath copy-on-descend fix. +func TestArraySetNodeConcurrentNoRace(t *testing.T) { + g := Graph{ + ID: "g", Name: "n", + StateVars: []StateVar{{Name: "grid", Type: "array", Initial: "[[0,0],[0,0]]", Sizing: "dynamic"}}, + Nodes: []Node{ + {ID: "t", Kind: "trigger.timer", Params: map[string]string{"interval": "1000"}}, + {ID: "s", Kind: "action.array.set", Params: map[string]string{"array": "grid", "index": "0,0", "expr": "1"}}, + }, + Wires: []Wire{{From: "t", To: "s"}}, + } + cg := compile(g) + cg.engine = &Engine{} + cg.genCtx = context.Background() + R := func(ds, name string) Value { + if ds == "local" { + return cg.getLocal(name) + } + return 0.0 + } + var wg sync.WaitGroup + for i := 0; i < 32; i++ { + wg.Add(2) + go func() { defer wg.Done(); cg.follow("t", "out", &runCtx{fired: "t", resolve: R}) }() + go func() { + defer wg.Done() + // Read the inner element concurrently; with the in-place setPath bug the + // writer mutates this same sub-array backing, producing a data race. + if arr, ok := cg.getLocal("grid").([]Value); ok && len(arr) > 0 { + if sub, ok := arr[0].([]Value); ok && len(sub) > 0 { + _ = sub[0] + } + } + }() + } + wg.Wait() +} + func TestCompileInitsLocalsFromDecls(t *testing.T) { g := Graph{ ID: "g", Name: "n",