[9.5](backport #7794) refactor: clone ParsedPolicy in processPolicy to prevent shared-state races - #7801
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
|
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 |
TL;DRBuildkite failed because this backport branch still contains unresolved merge-conflict markers in Remediation
Investigation detailsRoot CauseThis is a code bug introduced by an incomplete conflict resolution during backporting. The PR diff itself shows conflict markers still present in
Because of those markers, the Go compiler/parser errors immediately before any functional tests can run. Evidence
Verification
Follow-up
What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Conflict markers from the Mergify auto-backport of #7794 are resolved: - handleCheckin.go: use pp.Policy.Data.Outputs (9.5 Prepare has no options param) - handleCheckin_test.go: call processPolicy with pp.Clone() (no nil collector arg) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
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.