From 6e667518d842bfef9e17c3f4e1f4e1dca00bd662 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 01:03:02 +0000 Subject: [PATCH 1/9] docs(data): narrow ContextProvider thread-safety claim + add race test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #93 (Supersedes #126, which was filed as a #101 refile and covers the same ground; closed as duplicate of #93.) README.md advertised "Thread-safe Data Management" as a feature, but the implementation of ContextProvider.AddDataToContext only guarantees safety under the per-request derived-context pattern. The shallow maps.Copy of the existing top-level map (line 66) leaves nested-map references shared with the parent, and the recursive mergeIntoMap mutates those nested maps in place — so concurrent enrichment of a shared parent that already contains nested maps races. This PR narrows the documented contract to match the implementation, without changing behavior: - README.md:20 — replaced the broad "Thread-safe Data Management" bullet with "Context-derived Data" + a forward link to the new Concurrency subsection. - README.md ContextProvider section — new "Concurrency and thread-safety" subsection spelling out the per-request derived-context pattern as the safe usage, and explicitly carving out the unsafe pattern (multiple goroutines enriching a shared parent with nested maps). - platform/data/contextProvider.go — added a "# Concurrency" paragraph to the AddDataToContext godoc explaining the same contract, with a pointer to maps.Copy + mergeIntoMap as the technical reason. Tests: - platform/data/contextProvider_test.go — new TestContextProvider_AddDataToContext_Concurrent that exercises the documented safe pattern: 32 goroutines × 100 iterations, each threading its own derived context, with a per-iteration read-back invariant. Passes cleanly under -race. Stress: -count=20 -race on the new test, 20 runs, no flakes. Out of scope: actually fixing the nested-map race (e.g. deep-copy in maps.Copy or storing a sync container in the context value). The issue's two options were doc-narrow vs. code-fix; we chose doc-narrow. The code-fix path is left as a future option if the shared-parent pattern becomes a real-world need. --- README.md | 10 +++++- platform/data/contextProvider.go | 18 ++++++++++ platform/data/contextProvider_test.go | 49 +++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index a6f47c0..5c65df1 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ The top-level package (`polyscript.go`) exposes a single generic constructor — - **Unified Abstraction API**: Common interfaces and implementations for several scripting languages - **Flexible Engine Selection**: Easily switch between different script engines -- **Thread-safe Data Management**: Multiple ways to provide input data to scripts +- **Context-derived Data**: Per-request data threads through Go's `context.Context` via a derived-context pattern that's safe across goroutines when each request owns its own chain (see [Concurrency](#concurrency-and-thread-safety)). - **Compilation, Evaluation, and Data Handling**: Compile scripts once with static data when creating the evaluator instance, then run multiple evaluation executions with variable runtime input. ## Engines Implemented @@ -197,6 +197,14 @@ enrichedCtx, _ := evaluator.AddDataToContext(context.Background(), runtimeData) result, _ := evaluator.Eval(enrichedCtx) ``` +#### Concurrency and thread-safety + +`ContextProvider.AddDataToContext` is safe for the **per-request derived-context** pattern: each goroutine starts from its own context and threads the returned context forward for any subsequent enrichment. Multiple goroutines using independent context chains do not interfere with each other. + +Concurrent enrichment of a **shared parent context that already contains nested maps** is *not* safe. `AddDataToContext` shallow-copies the existing top-level map when deriving the new context; nested maps inside that map are reference-shared with the parent and are mutated in place during recursive merge. Two goroutines enriching such a shared parent with overlapping nested keys will race. + +If your usage looks like "fan out N goroutines, each enriches the same base context with its own slice of data," give each goroutine its own root context (e.g. `context.Background()` or `t.Context()`) and merge results at a join point on the caller side, rather than sharing a base context that has already been enriched with nested data. + ### Combining Static and Dynamic Runtime Data Use the following pattern for fixed configuration values and per-request data. Initial loading, parsing, and instantiating the script is relatively slow, so the example below shows how to set up the script once with static data and then reuse it many times with dynamic runtime data. diff --git a/platform/data/contextProvider.go b/platform/data/contextProvider.go index 99a5c8c..d1e10d9 100644 --- a/platform/data/contextProvider.go +++ b/platform/data/contextProvider.go @@ -50,6 +50,24 @@ func (p *ContextProvider) GetData(ctx context.Context) (map[string]any, error) { // and later values override earlier ones for duplicate keys. // // See README.md for detailed usage examples. +// +// # Concurrency +// +// Each caller MUST use the returned context for any subsequent +// enrichment. The supported pattern is one derived-context chain per +// request: independent chains do not share mutable state and are safe +// to operate on concurrently. +// +// Concurrent calls from goroutines that share a parent context already +// populated with nested maps are NOT safe. The top-level map is +// shallow-copied via [maps.Copy], so nested-map references survive into +// the new derived context. The recursive merge in [mergeIntoMap] +// mutates those nested maps in place, which races if another goroutine +// is concurrently reading or merging the same parent. +// +// If you need to enrich a single context from multiple goroutines, give +// each goroutine its own root context and merge results at a host-side +// join point — do not share an already-enriched parent. func (p *ContextProvider) AddDataToContext( ctx context.Context, data ...map[string]any, diff --git a/platform/data/contextProvider_test.go b/platform/data/contextProvider_test.go index 0a26721..4ddb83f 100644 --- a/platform/data/contextProvider_test.go +++ b/platform/data/contextProvider_test.go @@ -2,7 +2,9 @@ package data import ( "context" + "fmt" "net/http" + "sync" "testing" "github.com/robbyt/go-polyscript/platform/constants" @@ -583,3 +585,50 @@ func TestContextProvider_DataIntegration(t *testing.T) { assert.Contains(t, err.Error(), "empty keys are not allowed") }) } + +// TestContextProvider_AddDataToContext_Concurrent exercises the documented +// per-request derived-context pattern under -race: each goroutine starts +// from its own root context, threads the returned context through repeated +// AddDataToContext calls, and reads its own data back. Independent chains +// must not interfere — this test asserts the contract narrowed in #93's +// godoc and README updates. +func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { + t.Parallel() + + p := NewContextProvider(constants.EvalData) + + const goroutines = 32 + const iterations = 100 + + var wg sync.WaitGroup + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + go func(id int) { + defer wg.Done() + + // Per-request derived ctx — the documented safe pattern. + ctx := t.Context() + for j := 0; j < iterations; j++ { + key := fmt.Sprintf("g%d-iter%d", id, j) + next, err := p.AddDataToContext(ctx, map[string]any{key: j}) + if err != nil { + t.Errorf("goroutine %d iter %d: %v", id, j, err) + return + } + // Read-back invariant: our derived chain must contain our key. + got, err := p.GetData(next) + if err != nil { + t.Errorf("goroutine %d iter %d GetData: %v", id, j, err) + return + } + if got[key] != j { + t.Errorf("goroutine %d iter %d: got[%q] = %v, want %d", + id, j, key, got[key], j) + return + } + ctx = next + } + }(i) + } + wg.Wait() +} From d347335571d116b8aee020cf9b91e1c1ba0139a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 01:05:51 +0000 Subject: [PATCH 2/9] test(data): use Go 1.22+ for-range loop idiom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sonar flagged 1 new issue on the previous commit. Without dashboard access to see the exact rule, the most likely culprit is the pre-1.22-style `for i := 0; i < N; i++` counter pattern when `for range N` works (the project's on Go 1.26). Rewrite both loops in TestContextProvider_AddDataToContext_Concurrent: - `for i := 0; i < goroutines; i++` → `for id := range goroutines` - `for j := 0; j < iterations; j++` → `for j := range iterations` Also drop the redundant `(id int)` closure parameter passing — Go 1.22+ per-iteration loop variable semantics make capture-by-reference safe in the goroutine. Behavior unchanged. Re-ran `-race -count=5`, green. --- platform/data/contextProvider_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/platform/data/contextProvider_test.go b/platform/data/contextProvider_test.go index 4ddb83f..ba4f524 100644 --- a/platform/data/contextProvider_test.go +++ b/platform/data/contextProvider_test.go @@ -602,13 +602,13 @@ func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { var wg sync.WaitGroup wg.Add(goroutines) - for i := 0; i < goroutines; i++ { - go func(id int) { + for id := range goroutines { + go func() { defer wg.Done() // Per-request derived ctx — the documented safe pattern. ctx := t.Context() - for j := 0; j < iterations; j++ { + for j := range iterations { key := fmt.Sprintf("g%d-iter%d", id, j) next, err := p.AddDataToContext(ctx, map[string]any{key: j}) if err != nil { @@ -628,7 +628,7 @@ func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { } ctx = next } - }(i) + }() } wg.Wait() } From 3413ead5a50d679ce6863f4a03c893c17bd3f33a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 01:08:44 +0000 Subject: [PATCH 3/9] test(data): tighten test docstring to match shared-root reality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review flagged the docstring claiming "each goroutine starts from its own root context" while the code uses t.Context() (a shared cached root for the test). The test substance is correct — independent derived-context chains off an un-enriched root never race — but the comment overstated isolation. Reword to: "each goroutine builds its own derived-context chain off a shared, un-enriched root". This matches what the code actually does and matches the README's distinction between unsafe (shared enriched parent with nested maps) and safe (per-request derived chain off any un-enriched starting point). --- platform/data/contextProvider_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/platform/data/contextProvider_test.go b/platform/data/contextProvider_test.go index ba4f524..7014ee0 100644 --- a/platform/data/contextProvider_test.go +++ b/platform/data/contextProvider_test.go @@ -587,10 +587,12 @@ func TestContextProvider_DataIntegration(t *testing.T) { } // TestContextProvider_AddDataToContext_Concurrent exercises the documented -// per-request derived-context pattern under -race: each goroutine starts -// from its own root context, threads the returned context through repeated -// AddDataToContext calls, and reads its own data back. Independent chains -// must not interfere — this test asserts the contract narrowed in #93's +// safe concurrency pattern under -race: each goroutine builds its own +// derived-context chain off a shared, un-enriched root (t.Context()), +// threads the returned context through repeated AddDataToContext calls, +// and reads its own data back. The root carries no nested maps, so the +// chains never race on shared inner storage — independent chains must +// not interfere. This test asserts the contract narrowed in #93's // godoc and README updates. func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { t.Parallel() @@ -606,7 +608,7 @@ func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { go func() { defer wg.Done() - // Per-request derived ctx — the documented safe pattern. + // Shared un-enriched root; per-goroutine derived chain on top. ctx := t.Context() for j := range iterations { key := fmt.Sprintf("g%d-iter%d", id, j) From 68091697077b194d1218408be0df5049cf0fa333 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 01:34:02 +0000 Subject: [PATCH 4/9] test(data): use testify assert.*f helpers in the concurrent test Replace the hand-rolled `if err != nil { t.Errorf(); return }` blocks with the testify `*f` variants (assert.NoErrorf, assert.Equalf), which take a real format string instead of the msgAndArgs variadic. Pattern is `if !assert.NoErrorf(...) { return }` to preserve the early-return on first failure inside the goroutine. This keeps the test goroutine-safe (assert.* uses t.Errorf under the hood, which is fine from non-test goroutines, unlike require.*) while losing the hand-rolled error-message duplication and gaining the typed mismatch reporting from assert.Equalf. Behavior unchanged. Verified under -race -count=5. --- platform/data/contextProvider_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/platform/data/contextProvider_test.go b/platform/data/contextProvider_test.go index 7014ee0..9af64d6 100644 --- a/platform/data/contextProvider_test.go +++ b/platform/data/contextProvider_test.go @@ -612,20 +612,18 @@ func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { ctx := t.Context() for j := range iterations { key := fmt.Sprintf("g%d-iter%d", id, j) + next, err := p.AddDataToContext(ctx, map[string]any{key: j}) - if err != nil { - t.Errorf("goroutine %d iter %d: %v", id, j, err) + if !assert.NoErrorf(t, err, "goroutine %d iter %d", id, j) { return } + // Read-back invariant: our derived chain must contain our key. got, err := p.GetData(next) - if err != nil { - t.Errorf("goroutine %d iter %d GetData: %v", id, j, err) + if !assert.NoErrorf(t, err, "goroutine %d iter %d GetData", id, j) { return } - if got[key] != j { - t.Errorf("goroutine %d iter %d: got[%q] = %v, want %d", - id, j, key, got[key], j) + if !assert.Equalf(t, j, got[key], "goroutine %d iter %d key=%q", id, j, key) { return } ctx = next From 97c448227e4e180268ecfa6a00ca0bd6d70972f8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 02:11:06 +0000 Subject: [PATCH 5/9] fix(data): deep-copy nested maps in AddDataToContext (#93) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expands the scope of the doc-narrow change to actually fix the bug rather than document it. # Root cause AddDataToContext shallow-copied the parent context's top-level map into a fresh `toStore` via `maps.Copy`, then recursively merged incoming data via `mergeIntoMap`. The recursion at line 138-141 mutates nested maps in place — and because `maps.Copy` is shallow, those nested maps are the SAME map references held by the parent context. Two goroutines enriching the same parent with overlapping nested keys raced on shared inner storage, producing either a `-race` data-race report or a runtime `fatal error: concurrent map writes`. # Fix New `deepCopyMap` helper allocates a fresh `map[string]any` at every nesting level. AddDataToContext uses it in place of the prior shallow `maps.Copy`. The recursive merge now operates exclusively on freshly-allocated maps owned by the derived context — the parent's storage is never reached. Non-map values (slices, structs, *http.Request) are passed through by reference. ContextProvider never mutates them in place after storing, and the godoc + README now state that callers must not either. This matches existing behavior. # Test `TestContextProvider_AddDataToContext_Concurrent` is now a real regression test: 32 goroutines each enriching the SAME nested key on a SHARED already-enriched parent context. Without the fix, this test fires `-race` and panics with "concurrent map writes". With the fix in place, it passes cleanly under `-race -count=20`. The test also asserts the parent context is unchanged after concurrent enrichment, locking in the independence contract. Negative check performed locally: temporarily reverting just contextProvider.go to the shallow-copy version while keeping the new test makes the test panic on the first goroutine — confirms the test is genuine regression coverage, not a vacuous pass. # Docs The README + godoc Concurrency notes added in earlier commits are walked back from scary caveats to confident positive statements. The "Thread-safe Data Management" features bullet is restored (with a qualifier about deep-copying nested maps so the claim is honest). # What stays - The `processValue` deep-copy of incoming data was already correct and unchanged. - The race test's earlier shape (un-enriched parent) is subsumed by the strengthened shared-enriched-parent shape. - Non-map values still pass through by reference; that's noted explicitly in the README + godoc. Closes #93. --- README.md | 15 +++++-- platform/data/contextProvider.go | 54 ++++++++++++++++-------- platform/data/contextProvider_test.go | 59 +++++++++++++++++++++------ 3 files changed, 95 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index 5c65df1..ce35714 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ The top-level package (`polyscript.go`) exposes a single generic constructor — - **Unified Abstraction API**: Common interfaces and implementations for several scripting languages - **Flexible Engine Selection**: Easily switch between different script engines -- **Context-derived Data**: Per-request data threads through Go's `context.Context` via a derived-context pattern that's safe across goroutines when each request owns its own chain (see [Concurrency](#concurrency-and-thread-safety)). +- **Thread-safe Data Management**: Per-request data flows through Go's `context.Context` via a derived-context pattern. Each `AddDataToContext` call returns a context independent of its parent (nested maps are deep-copied), so concurrent enrichments from goroutines that share a parent never race — see [Concurrency](#concurrency-and-thread-safety). - **Compilation, Evaluation, and Data Handling**: Compile scripts once with static data when creating the evaluator instance, then run multiple evaluation executions with variable runtime input. ## Engines Implemented @@ -199,11 +199,18 @@ result, _ := evaluator.Eval(enrichedCtx) #### Concurrency and thread-safety -`ContextProvider.AddDataToContext` is safe for the **per-request derived-context** pattern: each goroutine starts from its own context and threads the returned context forward for any subsequent enrichment. Multiple goroutines using independent context chains do not interfere with each other. +`ContextProvider.AddDataToContext` is safe for concurrent use. Each call returns a derived context independent of its parent: data merged into the derived context never affects the parent or any sibling derived chain. -Concurrent enrichment of a **shared parent context that already contains nested maps** is *not* safe. `AddDataToContext` shallow-copies the existing top-level map when deriving the new context; nested maps inside that map are reference-shared with the parent and are mutated in place during recursive merge. Two goroutines enriching such a shared parent with overlapping nested keys will race. +Existing data from the parent context is deep-copied at every nesting level when forming the derived context, so concurrent enrichments from goroutines that share a parent context — even one already populated with nested maps — do not race on shared inner storage. -If your usage looks like "fan out N goroutines, each enriches the same base context with its own slice of data," give each goroutine its own root context (e.g. `context.Background()` or `t.Context()`) and merge results at a join point on the caller side, rather than sharing a base context that has already been enriched with nested data. +The typical pattern is one derived-context chain per request, with the returned context threaded forward for any subsequent enrichment: + +```go +ctx, _ := provider.AddDataToContext(parent, requestData) +ctx, _ = provider.AddDataToContext(ctx, moreData) // build on the returned ctx +``` + +Non-map values stored via `AddDataToContext` (slices, structs, `*http.Request`) are passed through by reference rather than copied. Callers must not mutate those values after handing them in; the provider itself never does. ### Combining Static and Dynamic Runtime Data diff --git a/platform/data/contextProvider.go b/platform/data/contextProvider.go index d1e10d9..3706741 100644 --- a/platform/data/contextProvider.go +++ b/platform/data/contextProvider.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "maps" "net/http" "github.com/robbyt/go-polyscript/internal/helpers" @@ -53,21 +52,18 @@ func (p *ContextProvider) GetData(ctx context.Context) (map[string]any, error) { // // # Concurrency // -// Each caller MUST use the returned context for any subsequent -// enrichment. The supported pattern is one derived-context chain per -// request: independent chains do not share mutable state and are safe -// to operate on concurrently. +// AddDataToContext is safe for concurrent use. Each call returns a +// derived context independent of its parent: data merged into the +// derived context does not affect the parent or any sibling derived +// chain. Existing data from the parent context is deep-copied at every +// nesting level when forming the derived context, so concurrent +// enrichments from goroutines that share a parent never race on shared +// inner map storage. (Non-map values such as slices, structs, and +// *http.Request are passed through by reference; callers must not +// mutate those after handing them to AddDataToContext.) // -// Concurrent calls from goroutines that share a parent context already -// populated with nested maps are NOT safe. The top-level map is -// shallow-copied via [maps.Copy], so nested-map references survive into -// the new derived context. The recursive merge in [mergeIntoMap] -// mutates those nested maps in place, which races if another goroutine -// is concurrently reading or merging the same parent. -// -// If you need to enrich a single context from multiple goroutines, give -// each goroutine its own root context and merge results at a host-side -// join point — do not share an already-enriched parent. +// The typical pattern is one derived-context chain per request, with +// the returned context threaded forward for any subsequent enrichment. func (p *ContextProvider) AddDataToContext( ctx context.Context, data ...map[string]any, @@ -81,7 +77,10 @@ func (p *ContextProvider) AddDataToContext( if existingData := ctx.Value(p.contextKey); existingData != nil { if existingMap, ok := existingData.(map[string]any); ok { - maps.Copy(toStore, existingMap) + // Deep-copy nested maps so subsequent in-place merge into + // toStore cannot race with goroutines reading or merging the + // same parent context. See the # Concurrency note above. + toStore = deepCopyMap(existingMap) } } @@ -165,3 +164,26 @@ func (p *ContextProvider) mergeIntoMap( // Non-map values simply replace existing values target[key] = value } + +// deepCopyMap returns a recursive copy of m. Nested map[string]any values +// are allocated fresh at every level so the returned map shares no +// mutable map storage with m. Other values (slices, structs, primitives, +// *http.Request) are passed through by reference — ContextProvider never +// mutates them in place after storing, but callers must not mutate them +// either. +// +// Returns nil when m is nil. +func deepCopyMap(m map[string]any) map[string]any { + if m == nil { + return nil + } + out := make(map[string]any, len(m)) + for k, v := range m { + if nested, ok := v.(map[string]any); ok { + out[k] = deepCopyMap(nested) + } else { + out[k] = v + } + } + return out +} diff --git a/platform/data/contextProvider_test.go b/platform/data/contextProvider_test.go index 9af64d6..884467f 100644 --- a/platform/data/contextProvider_test.go +++ b/platform/data/contextProvider_test.go @@ -586,19 +586,29 @@ func TestContextProvider_DataIntegration(t *testing.T) { }) } -// TestContextProvider_AddDataToContext_Concurrent exercises the documented -// safe concurrency pattern under -race: each goroutine builds its own -// derived-context chain off a shared, un-enriched root (t.Context()), -// threads the returned context through repeated AddDataToContext calls, -// and reads its own data back. The root carries no nested maps, so the -// chains never race on shared inner storage — independent chains must -// not interfere. This test asserts the contract narrowed in #93's -// godoc and README updates. +// TestContextProvider_AddDataToContext_Concurrent is the regression test +// for the deep-copy fix in #93. Goroutines share an already-enriched +// parent context whose value contains a nested map. Each goroutine +// enriches the SAME nested key with its own data and reads it back. +// Before the deep-copy fix, the recursive merge mutated the nested map +// in-place — concurrent goroutines raced on shared inner storage and +// -race would fire. The fix deep-copies nested maps when deriving from +// the parent so each derived chain owns its own nested map storage. +// +// 32 goroutines × 100 iterations = 3200 enrichment + read-back checks +// against a shared parent. Passes cleanly under -race. func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { t.Parallel() p := NewContextProvider(constants.EvalData) + // Pre-enrich a shared root with a nested map. This is the exact + // shape that was unsafe under the old shallow-copy implementation. + parent, err := p.AddDataToContext(t.Context(), map[string]any{ + "shared": map[string]any{"baseline": "from-parent"}, + }) + require.NoError(t, err) + const goroutines = 32 const iterations = 100 @@ -608,22 +618,36 @@ func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { go func() { defer wg.Done() - // Shared un-enriched root; per-goroutine derived chain on top. - ctx := t.Context() + // All goroutines start from the SAME enriched parent. + ctx := parent for j := range iterations { key := fmt.Sprintf("g%d-iter%d", id, j) - next, err := p.AddDataToContext(ctx, map[string]any{key: j}) + // Enrich the nested "shared" map with our own key. Before + // the fix this was the racing path: mergeIntoMap recursed + // into parent.shared and mutated it concurrently. + next, err := p.AddDataToContext(ctx, map[string]any{ + "shared": map[string]any{key: j}, + }) if !assert.NoErrorf(t, err, "goroutine %d iter %d", id, j) { return } - // Read-back invariant: our derived chain must contain our key. + // Read-back invariant: our derived chain must contain + // our key under "shared", plus the baseline from parent. got, err := p.GetData(next) if !assert.NoErrorf(t, err, "goroutine %d iter %d GetData", id, j) { return } - if !assert.Equalf(t, j, got[key], "goroutine %d iter %d key=%q", id, j, key) { + shared, ok := got["shared"].(map[string]any) + if !assert.Truef(t, ok, "goroutine %d iter %d: shared not a map", id, j) { + return + } + if !assert.Equalf(t, j, shared[key], "goroutine %d iter %d shared[%q]", id, j, key) { + return + } + if !assert.Equalf(t, "from-parent", shared["baseline"], + "goroutine %d iter %d: parent baseline lost", id, j) { return } ctx = next @@ -631,4 +655,13 @@ func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) { }() } wg.Wait() + + // Parent context must be unchanged after all the concurrent + // enrichment — the deep-copy fix guarantees independence. + parentData, err := p.GetData(parent) + require.NoError(t, err) + parentShared, ok := parentData["shared"].(map[string]any) + require.True(t, ok) + require.Equal(t, "from-parent", parentShared["baseline"]) + require.Len(t, parentShared, 1, "parent.shared must hold only the baseline key; got %v", parentShared) } From fd1de748259126e25538dc0a9de6712b2d3ea1c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 03:32:46 +0000 Subject: [PATCH 6/9] fix(data): tighten deepCopyMap nil-handling + reference-type wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review caught five things on the prior commit. Triage: 1. README L20 — PR title/body claimed "doc-only" while latest commit is a behavior change. Fixed: PR title + body updated to reflect the actual scope (fix + docs + test, not just docs). 2. README L213 + contextProvider.go L63 godoc — wording listed slices, structs, and *http.Request as "reference types passed through by reference". Inaccurate: structs are value types in Go (copied when stored in `any`); *http.Request is converted to a map via helpers.RequestToMap during processValue and never stored as a pointer. Reworded to focus on actual reference types (slices, pointers, channels, function values) and clarify the *http.Request conversion path. 3. test L624 — Copilot claimed `for id := range goroutines` "won't compile and won't capture id". False positive: Go 1.22 added range-over-integer and per-iteration loop-var semantics. Test was verified to run under -race -count=20 + a negative check. 4. contextProvider.go L83 — real bug. deepCopyMap returned nil for nil input; a typed-nil map[string]any stored as a context value (existingData.(map[string]any) succeeds with a nil concrete value) would set toStore to nil, then the subsequent mergeIntoMap call would panic with "assignment to entry in nil map". Fixed: dropped the early-return-nil; deepCopyMap now always returns make(...), which is correct for both nil and non-nil inputs because len(nil) == 0 and range nil_map is a no-op. 5. New test TestContextProvider_AddDataToContext_TypedNilParent locks in the fix from (4): stores a typed-nil map[string]any in the context, then calls AddDataToContext and confirms no panic + the read-back works. Behavior change: the deep-copy fix from the previous commit is retained; this commit makes deepCopyMap robust to typed-nil parents and corrects the documentation accuracy. --- README.md | 2 +- platform/data/contextProvider.go | 34 +++++++++++++++++---------- platform/data/contextProvider_test.go | 24 +++++++++++++++++++ 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index ce35714..6f08a0d 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ ctx, _ := provider.AddDataToContext(parent, requestData) ctx, _ = provider.AddDataToContext(ctx, moreData) // build on the returned ctx ``` -Non-map values stored via `AddDataToContext` (slices, structs, `*http.Request`) are passed through by reference rather than copied. Callers must not mutate those values after handing them in; the provider itself never does. +Reference-typed values stored in the map (slices, pointers, channels, function values) remain shared with the caller after `AddDataToContext` returns. The provider never mutates them, but callers should avoid mutating them either. Value types (int, string, struct, etc.) are copied as usual when stored in an `any` slot. `*http.Request` and `http.Request` inputs are converted to maps via `helpers.RequestToMap` on the way in, so the underlying request itself is not retained. ### Combining Static and Dynamic Runtime Data diff --git a/platform/data/contextProvider.go b/platform/data/contextProvider.go index 3706741..9a47124 100644 --- a/platform/data/contextProvider.go +++ b/platform/data/contextProvider.go @@ -58,9 +58,16 @@ func (p *ContextProvider) GetData(ctx context.Context) (map[string]any, error) { // chain. Existing data from the parent context is deep-copied at every // nesting level when forming the derived context, so concurrent // enrichments from goroutines that share a parent never race on shared -// inner map storage. (Non-map values such as slices, structs, and -// *http.Request are passed through by reference; callers must not -// mutate those after handing them to AddDataToContext.) +// inner map storage. +// +// Reference-typed values stored in the map (slices, pointers, channels, +// function values) remain shared with the source — ContextProvider +// never mutates them in place, but callers should avoid mutating them +// after handing them to AddDataToContext. Value types (int, string, +// struct, etc.) are copied as usual when stored in an `any` slot. +// *http.Request and http.Request inputs are converted to maps via +// helpers.RequestToMap on the way in, so the underlying request is not +// stored. // // The typical pattern is one derived-context chain per request, with // the returned context threaded forward for any subsequent enrichment. @@ -166,17 +173,20 @@ func (p *ContextProvider) mergeIntoMap( } // deepCopyMap returns a recursive copy of m. Nested map[string]any values -// are allocated fresh at every level so the returned map shares no -// mutable map storage with m. Other values (slices, structs, primitives, -// *http.Request) are passed through by reference — ContextProvider never -// mutates them in place after storing, but callers must not mutate them -// either. +// are allocated fresh at every level so the returned map shares no mutable +// map storage with m. +// +// Always returns a non-nil map, even when m is nil or empty; this lets +// AddDataToContext use the result directly without a separate guard +// against a nil parent-context value. // -// Returns nil when m is nil. +// Non-map values are stored by assignment to an `any` slot. Value types +// (int, string, struct, etc.) are copied as usual; reference types +// (slices, pointers, channels, function values) remain shared with the +// source. ContextProvider never mutates stored values in place; callers +// should avoid mutating reference-typed values after handing them to +// AddDataToContext. func deepCopyMap(m map[string]any) map[string]any { - if m == nil { - return nil - } out := make(map[string]any, len(m)) for k, v := range m { if nested, ok := v.(map[string]any); ok { diff --git a/platform/data/contextProvider_test.go b/platform/data/contextProvider_test.go index 884467f..5f21070 100644 --- a/platform/data/contextProvider_test.go +++ b/platform/data/contextProvider_test.go @@ -586,6 +586,30 @@ func TestContextProvider_DataIntegration(t *testing.T) { }) } +// TestContextProvider_AddDataToContext_TypedNilParent guards against a +// regression caught in Copilot review of #93: if a typed-nil map[string]any +// is stored in the context value (e.g. `context.WithValue(parent, key, +// (map[string]any)(nil))`), the parent map ranges as empty, deepCopyMap +// must still produce a usable non-nil destination, and the subsequent +// merge must not panic with "assignment to entry in nil map". +func TestContextProvider_AddDataToContext_TypedNilParent(t *testing.T) { + t.Parallel() + p := NewContextProvider(constants.EvalData) + + var typedNil map[string]any + parent := context.WithValue(t.Context(), constants.EvalData, typedNil) + + // This call would have panicked under the early-return-nil version + // of deepCopyMap (toStore = nil → mergeIntoMap → assignment to entry + // in nil map). + next, err := p.AddDataToContext(parent, map[string]any{"k": "v"}) + require.NoError(t, err) + + got, err := p.GetData(next) + require.NoError(t, err) + assert.Equal(t, "v", got["k"]) +} + // TestContextProvider_AddDataToContext_Concurrent is the regression test // for the deep-copy fix in #93. Goroutines share an already-enriched // parent context whose value contains a nested map. Each goroutine From cf743bf8074d99ba7e3064fd9d2b1a63610cb156 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 05:46:27 +0000 Subject: [PATCH 7/9] docs(data): clarify deep-copy is map[string]any-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review (3rd pass) caught a real precision gap in the concurrency docs: `processValue` and `deepCopyMap` only recurse into `map[string]any` values. Other map types (`map[string]string`, `map[any]any`, custom map type aliases) hit the default `case` and pass through unchanged — they're stored as opaque values, by reference. Update both the README "Concurrency and thread-safety" subsection and the AddDataToContext godoc to: - List maps (other than map[string]any) alongside other reference types that remain shared with the source. - State explicitly that the deep-copy guarantee is map[string]any- specific, with the processValue / deepCopyMap call paths cited. The third Copilot comment on this round (line 643 test "won't compile, for id := range goroutines doesn't support range over int") is the same Go-1.22-range-int false positive as the previous two rounds. Verified compiling + running by `-race -count=20`. Skipped. --- README.md | 2 +- platform/data/contextProvider.go | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 6f08a0d..c7445ac 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ ctx, _ := provider.AddDataToContext(parent, requestData) ctx, _ = provider.AddDataToContext(ctx, moreData) // build on the returned ctx ``` -Reference-typed values stored in the map (slices, pointers, channels, function values) remain shared with the caller after `AddDataToContext` returns. The provider never mutates them, but callers should avoid mutating them either. Value types (int, string, struct, etc.) are copied as usual when stored in an `any` slot. `*http.Request` and `http.Request` inputs are converted to maps via `helpers.RequestToMap` on the way in, so the underlying request itself is not retained. +Reference-typed values stored in the map (slices, pointers, channels, function values, and maps **other than** `map[string]any`) remain shared with the caller after `AddDataToContext` returns. The provider never mutates them, but callers should avoid mutating them either. The deep-copy guarantee applies specifically to `map[string]any` values — `processValue` and `deepCopyMap` recurse into those; other map types (e.g. `map[string]string`, `map[any]any`) are passed through as opaque values. Value types (int, string, struct, etc.) are copied as usual when stored in an `any` slot. `*http.Request` and `http.Request` inputs are converted to `map[string]any` via `helpers.RequestToMap` on the way in, so the underlying request itself is not retained. ### Combining Static and Dynamic Runtime Data diff --git a/platform/data/contextProvider.go b/platform/data/contextProvider.go index 9a47124..ef80ae4 100644 --- a/platform/data/contextProvider.go +++ b/platform/data/contextProvider.go @@ -61,11 +61,15 @@ func (p *ContextProvider) GetData(ctx context.Context) (map[string]any, error) { // inner map storage. // // Reference-typed values stored in the map (slices, pointers, channels, -// function values) remain shared with the source — ContextProvider -// never mutates them in place, but callers should avoid mutating them -// after handing them to AddDataToContext. Value types (int, string, -// struct, etc.) are copied as usual when stored in an `any` slot. -// *http.Request and http.Request inputs are converted to maps via +// function values, and maps OTHER than map[string]any) remain shared +// with the source — ContextProvider never mutates them in place, but +// callers should avoid mutating them after handing them to +// AddDataToContext. The deep-copy guarantee applies specifically to +// map[string]any values, which processValue and deepCopyMap recurse +// into; other map types (e.g. map[string]string, map[any]any) are +// stored as opaque values. Value types (int, string, struct, etc.) are +// copied as usual when stored in an `any` slot. *http.Request and +// http.Request inputs are converted to map[string]any via // helpers.RequestToMap on the way in, so the underlying request is not // stored. // From 79a9343d293b6bceeacf32f965409dad656f3af4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 12:43:30 +0000 Subject: [PATCH 8/9] docs(README): move reference-types detail to godoc, link from README The detailed paragraph on which value kinds are deep-copied vs. passed by reference (slices/pointers/channels/funcs/non-map[string]any maps remain shared; *http.Request converted via RequestToMap; etc.) duplicated content that already lives in the AddDataToContext godoc. That level of implementation detail belongs near the code, not in a top-level README onboarding doc. Drop the duplicate from README's "Concurrency and thread-safety" subsection and replace it with a one-line pointer to the godoc. The remaining README content stays focused on the user-facing pattern: the safety guarantee plus the per-request derived-context example. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c7445ac..8a3c3cd 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ ctx, _ := provider.AddDataToContext(parent, requestData) ctx, _ = provider.AddDataToContext(ctx, moreData) // build on the returned ctx ``` -Reference-typed values stored in the map (slices, pointers, channels, function values, and maps **other than** `map[string]any`) remain shared with the caller after `AddDataToContext` returns. The provider never mutates them, but callers should avoid mutating them either. The deep-copy guarantee applies specifically to `map[string]any` values — `processValue` and `deepCopyMap` recurse into those; other map types (e.g. `map[string]string`, `map[any]any`) are passed through as opaque values. Value types (int, string, struct, etc.) are copied as usual when stored in an `any` slot. `*http.Request` and `http.Request` inputs are converted to `map[string]any` via `helpers.RequestToMap` on the way in, so the underlying request itself is not retained. +For the details of which value kinds are deep-copied versus stored by reference, see the [`AddDataToContext` godoc](https://pkg.go.dev/github.com/robbyt/go-polyscript/platform/data#ContextProvider.AddDataToContext). ### Combining Static and Dynamic Runtime Data From cbb4edb30c30f3afa37b56ad5243534843d41a9b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 12:48:10 +0000 Subject: [PATCH 9/9] docs(README): restore brief features bullet for thread-safe data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Robbyt review (PR #128 line 20): the expanded "Thread-safe Data Management" features bullet spilled implementation surface (derived-context pattern, AddDataToContext, nested-map deep-copy, goroutine-race semantics) into a top-level highlight where new users shouldn't yet care about the mechanism. Revert to the original brief one-liner — the Concurrency subsection deeper in the README and the AddDataToContext godoc carry the actual explanation for readers who get there. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8a3c3cd..dc2954c 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ The top-level package (`polyscript.go`) exposes a single generic constructor — - **Unified Abstraction API**: Common interfaces and implementations for several scripting languages - **Flexible Engine Selection**: Easily switch between different script engines -- **Thread-safe Data Management**: Per-request data flows through Go's `context.Context` via a derived-context pattern. Each `AddDataToContext` call returns a context independent of its parent (nested maps are deep-copied), so concurrent enrichments from goroutines that share a parent never race — see [Concurrency](#concurrency-and-thread-safety). +- **Thread-safe Data Management**: Multiple ways to provide input data to scripts - **Compilation, Evaluation, and Data Handling**: Compile scripts once with static data when creating the evaluator instance, then run multiple evaluation executions with variable runtime input. ## Engines Implemented