fix(data): deep-copy nested maps in ContextProvider.AddDataToContext#128
Open
robbyt wants to merge 9 commits into
Open
fix(data): deep-copy nested maps in ContextProvider.AddDataToContext#128robbyt wants to merge 9 commits into
robbyt wants to merge 9 commits into
Conversation
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.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR narrows the documented thread-safety guarantees for ContextProvider.AddDataToContext to match the current implementation (shallow top-level copy + in-place nested-map merge), and adds a race-oriented concurrency test that exercises the supported “derived-context chain per goroutine” usage.
Changes:
- Updates
README.mdto replace the broad “Thread-safe Data Management” claim with a more precise “Context-derived Data” feature description and a new “Concurrency and thread-safety” subsection. - Adds a
# Concurrencysection toContextProvider.AddDataToContextgodoc documenting the safe vs unsafe concurrency patterns and why nested maps are problematic. - Adds
TestContextProvider_AddDataToContext_Concurrentto validate the documented safe pattern under concurrent use (intended for-raceruns).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
README.md |
Narrows thread-safety claims and documents the safe/unsafe ContextProvider concurrency patterns. |
platform/data/contextProvider.go |
Adds explicit godoc contract for concurrency/thread-safety behavior. |
platform/data/contextProvider_test.go |
Adds a concurrent test to exercise the supported derived-context pattern under -race. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| go func() { | ||
| defer wg.Done() | ||
|
|
||
| // Per-request derived ctx — the documented safe pattern. |
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).
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.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
platform/data/contextProvider.go:173
- The
deepCopyMapdoc comment says “slices, structs, primitives, *http.Request are passed through by reference”, but structs and primitives are value types (copied when assigned to anany). Also*http.Requestvalues passed intoAddDataToContextare normally converted to maps; a*http.Requestwould only be present here if someone manually stored it in the context. Please tighten the wording to accurately describe which values can remain shared/mutable (typically reference types like slices/pointers) and when a*http.Requestwould actually be stored.
// 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.
| // 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) |
| 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. |
Comment on lines
+61
to
+63
| // 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.) |
robbyt
changed the title
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
platform/data/contextProvider_test.go:648
- This loop won’t compile:
for j := range iterationscan’t range over anint. Replace with a counted loop (e.g.,for j := 0; j < iterations; j++ { ... }).
for j := range iterations {
key := fmt.Sprintf("g%d-iter%d", id, j)
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.
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.
robbyt
commented
May 14, 2026
|
|
||
| - **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 |
Owner
Author
There was a problem hiding this comment.
@claude the previous text was better. This is too much detail for so early in the readme doc. It's overwhelming for new users to start talking about how things actually work this soon
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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a nested-map race in
ContextProvider.AddDataToContextand walks the docs forward to reflect the now-honest thread-safety contract.The bug
AddDataToContextshallow-copied the parent context's top-level map into a freshtoStoreviamaps.Copy, then recursively merged incoming data viamergeIntoMap. The recursive merge mutates nested maps in place — and becausemaps.Copyis shallow, those nested maps were 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-racedata-race report or a runtimefatal error: concurrent map writes.The fix
New
deepCopyMaphelper allocates a freshmap[string]anyat every nesting level.AddDataToContextuses it in place ofmaps.Copy. The recursive merge now operates exclusively on freshly-allocated maps owned by the derived context — the parent's storage is never reached.deepCopyMapalways returns a non-nil map (even for nil input), so a typed-nilmap[string]anystored as a context value can't lead to a "assignment to entry in nil map" panic.Non-map values are stored by assignment to an
anyslot. Value types (int, string, struct) are copied as usual; reference types (slices, pointers, channels, function values) remain shared with the source. ContextProvider never mutates stored values in place.*http.Requestandhttp.Requestinputs are converted to maps viahelpers.RequestToMapon the way in, so the underlying request is not retained.Tests
TestContextProvider_AddDataToContext_Concurrent— 32 goroutines × 100 iterations against a shared enriched parent (the pattern that used to race). Without the fix this panics withconcurrent map writeson the first iteration; with the fix it passes cleanly under-race -count=20. Also asserts the parent context is unchanged after concurrent enrichment, locking in the independence contract.TestContextProvider_AddDataToContext_TypedNilParent— guards against a regression caught in Copilot review: a typed-nilmap[string]anystored in the context must not panic the subsequent merge.Docs
*http.Requestis converted, not stored).ContextProvider.AddDataToContextgodoc — same.Supersedes
Test plan
go test -race -count=1 ./platform/data/...— green (new test + existing typed-nil regression test pass)go test -race -count=20 -run TestContextProvider_AddDataToContext_Concurrent— no flakescontextProvider.goto shallow-copy locally, kept the new test → panics withconcurrent map writeson the first goroutine, confirming the test is real regression coveragego test -race -count=1 ./...— full suite greengo vet ./...— cleanCloses #93
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL