[8.19](backport #7794) refactor: clone ParsedPolicy in processPolicy to prevent shared-state races - #7803
Conversation
… races (#7794) * refactor: clone ParsedPolicy at dispatch site to prevent shared-state races Move the ParsedPolicy clone from processPolicy to the monitor dispatch loop, establishing ownership at the point where the policy transitions from the shared monitor cache to an individual subscriber. Each channel send now transfers an exclusively-owned *ParsedPolicy to its subscriber, so processPolicy can treat its argument as its own. Changes: - monitor.go: compute cloned := policy.pp.Clone() before the select so it is not embedded in the send expression (Go evaluates all select case expressions on entry; doing clone inside the send ran it even when ctx.Done/default fired under the held mutex) - handleCheckin.go: processPolicy no longer needs to clone pp - handleCheckin_test.go: pass pp.Clone() per goroutine in the concurrent regression test, mirroring what the monitor now does at dispatch time - ParsedPolicy.Clone() deep-copies every slice, map, and pointer field: SecretKeys and Policy.Namespaces via slices.Clone; Inputs, Roles (including RoleT.Raw bytes), and Outputs (including Output.Role pointers) element by element; Agent and Fleet via maps.Clone; Policy.Data via model.ClonePolicyData - model.ClonePolicyData now also clones Agent/Fleet maps, OutputPermissions bytes, and output/OTel section maps via deepCloneMapAny — a new recursive helper that deep-clones map[string]any and []any trees; necessary because ProcessOutputSecret and prepareOTelExporters mutate nested map entries in-place; nil Inputs is preserved as nil - TestParsedPolicyCloneIsolation verifies that mutating each field of a clone does not affect the original; SecretKeys check mutates an existing element (not append) to detect shared backing storage; Outputs check mutates both top-level and nested (ssl.key) paths to catch shallow-clone regressions Relates #7739 Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix: skip Clone() when ctx is already cancelled in dispatchPending Guard policy.pp.Clone() with a ctx.Err() check so the potentially expensive clone does not run while holding m.mut when the context was already cancelled between m.limit.Wait and the clone site. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * refactor: narrow m.mut critical section in dispatchPending Hold m.mut only for queue/map operations (popFront, pushFront, policies lookup). Release it before m.limit.Wait, Clone(), and channel sends so Subscribe/Unsubscribe/updatePolicy are not blocked during those potentially slow operations. policyT is a value type so the map lookup copies the struct, making it safe to use policy.pp after the lock is released. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * refactor: use direct assignment for Agent/Fleet/Inputs in Clone() maps.Clone on Agent and Fleet, and per-element maps.Clone on Inputs, were shallow clones that implied deeper isolation than they provided. processPolicy never accesses Agent or Fleet, and never mutates Inputs elements — only overwrites the slice header via Policy.Data.Inputs. Replace with direct assignment (Agent, Fleet) and slices.Clone (Inputs) and document the invariant. Remove the now-incorrect Agent isolation assertion from TestParsedPolicyCloneIsolation. Also drops the unused "maps" import. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix: restore Agent/Fleet aliasing invariant in ParsedPolicy.Clone() NewParsedPolicy assigns pp.Agent = p.Data.Agent and pp.Fleet = p.Data.Fleet, so the two fields alias the same map. Clone() was leaving clone.Agent/Fleet pointing at the original's maps while clone.Policy.Data had freshly-cloned maps from ClonePolicyData, breaking the invariant. Set clone.Agent and clone.Fleet from the cloned Policy.Data after ClonePolicyData runs, preserving the alias on the clone side. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test: restore Agent isolation assertion after Clone() aliasing fix After 0f62096 clone.Agent is derived from the cloned Policy.Data (not a shared reference), so the stale comment is removed and the top-level isolation assertion is restored. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --------- Co-authored-by: Claude Sonnet 4.6 <[email protected]> (cherry picked from commit c907276) # Conflicts: # internal/pkg/api/handleCheckin.go # internal/pkg/api/handleCheckin_test.go # internal/pkg/model/ext.go # internal/pkg/policy/monitor.go # internal/pkg/policy/parsed_policy_test.go
|
Cherry-pick of c907276 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
This comment has been minimized.
This comment has been minimized.
Conflict markers from the Mergify auto-backport of #7794 resolved: - ext.go: use bytes.Clone for json.RawMessage Agent/Fleet (8.19 types), take nil Inputs and bytes.Clone(OutputPermissions), add deepCloneMapAny/ deepCloneSliceAny helpers; skip OTel fields absent from 8.19 PolicyData - parsed_policy.go: remove Agent/Fleet field assignments absent from 8.19 ParsedPolicy struct - handleCheckin.go: use pp.Policy.Data.Outputs directly (pp is a clone); update SecretKeys on pp; skip prepareOTelExporters (not in 8.19); keep &keys pointer for *[]string SecretPaths type in 8.19 API - handleCheckin_test.go: call processPolicy with pp.Clone() (keep HEAD goroutine structure; no nil collector arg) - monitor.go: take cherry-pick push-back on rate-limit error and ctx cancel - parsed_policy_test.go: add TestParsedPolicyCloneIsolation without Agent field references (absent from 8.19 ParsedPolicy); skip OTel/secrets tests Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
cloneOTelSection was introduced by the cherry-pick but is unused in 8.19 since PolicyData has no OTel fields. The extra blank line left by the conflict-resolution commit caused a goimports formatting failure. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
TL;DR
Remediation
Investigation detailsRoot CauseThis is a code-quality failure (formatting drift), not a runtime/test flake. The Evidence
VerificationNot run (detective workflow is read-only; analysis is based on the Buildkite artifact logs and PR metadata). Follow-upNo matching open What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Summary
ParsedPolicy.Clone()which returns a fully independent copy — every slice, map, and pointer field gets its own backing storage:SecretKeysandPolicy.Namespacesviaslices.Clone;Inputs,Roles(includingRoleT.Rawbytes), andOutputs(includingOutput.Rolepointers) deep-copied element by element;AgentandFleetviamaps.Clone;Policy.Dataviamodel.ClonePolicyData.model.ClonePolicyDatato clone itsAgentandFleetmaps,OutputPermissionsbytes, and OTel section maps viacloneOTelSection— necessary becauseprepareOTelExportersmutates per-component maps in-place;nilInputsis preserved asnil.monitor.go:s.ch <- policy.pp.Clone()instead of&policy.pp. Each channel send now transfers an exclusively-owned*ParsedPolicyto its subscriber, soprocessPolicyreceives a copy it already owns and can mutate freely — no implicit contract that callers must clone.processPolicyis unchanged in behaviour; the clone just moves upstream to the natural ownership-transfer boundary.TestParsedPolicyCloneIsolationto verify that mutating each field of a clone does not affect the original.References
Relates #7739
🤖 Generated with Claude Code
This is an automatic backport of pull request #7794 done by Mergify.