refactor: clone ParsedPolicy in processPolicy to prevent shared-state races

[ycombinator]

Summary

• Adds ParsedPolicy.Clone() which returns a fully independent copy — every slice, map, and pointer field gets its own backing storage: SecretKeys and Policy.Namespaces via slices.Clone; Inputs, Roles (including RoleT.Raw bytes), and Outputs (including Output.Role pointers) deep-copied element by element; Agent and Fleet via maps.Clone; Policy.Data via model.ClonePolicyData.
• Also updates model.ClonePolicyData to clone its Agent and Fleet maps, OutputPermissions bytes, and OTel section maps via cloneOTelSection — necessary because prepareOTelExporters mutates per-component maps in-place; nil Inputs is preserved as nil.
• Moves the clone to the dispatch site in monitor.go: s.ch <- policy.pp.Clone() instead of &policy.pp. Each channel send now transfers an exclusively-owned *ParsedPolicy to its subscriber, so processPolicy receives a copy it already owns and can mutate freely — no implicit contract that callers must clone.
• processPolicy is unchanged in behaviour; the clone just moves upstream to the natural ownership-transfer boundary.
• Adds TestParsedPolicyCloneIsolation to verify that mutating each field of a clone does not affect the original.

References

Relates #7739

🤖 Generated with Claude Code

[mergify]

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[Copilot]

🟡 Changes recommended

ParsedPolicy.Clone() currently copies pp.Policy without cloning Policy.Namespaces, so the implementation does not fully match its stated “independent backing storage” guarantee and may still allow shared-slice races if that field is ever mutated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors policy handling to eliminate shared-state mutation in processPolicy by introducing a ParsedPolicy.Clone() method and switching processPolicy to operate on a per-call cloned ParsedPolicy, preventing cross-agent goroutines from racing when preparing policy actions.

Changes:

• Added (*policy.ParsedPolicy).Clone() to copy ParsedPolicy slice/map fields into independent backing storage.
• Updated processPolicy to clone the ParsedPolicy up front and mutate pp.SecretKeys on the clone (removing the narrower secretKeys := slices.Clone(...) approach).

File summaries

File	Description
internal/pkg/policy/parsed_policy.go	Adds ParsedPolicy.Clone() to make per-goroutine copies of mutable slice/map fields.
internal/pkg/api/handleCheckin.go	Updates processPolicy to use pp = pp.Clone() and mutate secret key paths on the cloned policy instance.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

ParsedPolicy.Clone() currently still shares some backing storage/pointers despite claiming full isolation, which weakens the race-safety contract this PR is relying on.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

ParsedPolicy.Clone() still shallow-copies mutable nested maps (notably OTel exporter config maps), so processPolicy can still race when it mutates those configs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/pkg/policy/parsed_policy.go:76

• Clone() currently relies on model.ClonePolicyData, which shallow-clones OTel maps like Policy.Data.Exporters (only the outer map). prepareOTelExporters mutates each exporter config map in-place, so if exporter entries are map[string]any (as expected), those inner maps can still be shared across goroutines and race. Consider deep-cloning the exporter config maps (and other mutable Policy.Data maps if needed) inside Clone() to make the copy actually mutation-safe.

	clone.Policy.Data = model.ClonePolicyData(pp.Policy.Data)
	clone.Policy.Namespaces = slices.Clone(pp.Policy.Namespaces)

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new cloning still leaves some mutable policy data potentially shared (and lacks focused unit coverage for clone-independence), which can undermine the intended race-prevention guarantee.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/pkg/policy/parsed_policy.go:64

• ParsedPolicy.Clone is new concurrency-safety plumbing for processPolicy, but there are no unit tests validating that the clone is actually mutation-independent (e.g., SecretKeys slice and nested policy data such as OTel exporter configs). Adding a focused test would help prevent regressions in this area.

// Clone returns a copy of pp where every slice, map, and pointer field has its
// own backing storage, so concurrent goroutines can mutate their copy without racing.
func (pp *ParsedPolicy) Clone() *ParsedPolicy {

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

ParsedPolicy.Clone() currently leaves Agent/Fleet pointing at the original maps (while Policy.Data.Agent/Fleet are cloned), breaking aliasing invariants and retaining shared mutable state that undermines the intended ownership-transfer semantics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new clone-isolation test currently doesn’t reliably exercise nested-map sharing for outputs and includes a misleading comment, weakening regression protection for the concurrency fix.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/pkg/policy/parsed_policy_test.go:131

• The nested-map isolation assertion can be a no-op with the current embedded test_policy.json because its outputs don't include an existing nested map (e.g., no "ssl" object). In that case, the test only proves top-level output-map isolation and would still pass if ClonePolicyData shallow-cloned nested maps/slices. Seed a nested map in the original before cloning so the test reliably detects nested sharing regressions.

	// Seed a known secret key so we can detect cross-contamination.
	original.SecretKeys = []string{"outputs.default.token"}

	clone := original.Clone()

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes correctly establish per-subscriber ownership of mutable policy state, and include targeted tests to prevent regressions of the reported race.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

@Mergifyio backport 9.5 9.4 8.19

[mergify]

backport 9.5 9.4 8.19

✅ Backports have been created

Details

• #7801 [9.5](backport #7794) refactor: clone ParsedPolicy in processPolicy to prevent shared-state races has been created for branch 9.5 but encountered conflicts

Cherry-pick of c907276 has failed:

On branch mergify/bp/9.5/pr-7794
Your branch is up to date with 'origin/9.5'.

You are currently cherry-picking commit c907276.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   internal/pkg/model/ext.go
	modified:   internal/pkg/policy/monitor.go
	modified:   internal/pkg/policy/parsed_policy.go
	modified:   internal/pkg/policy/parsed_policy_test.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   internal/pkg/api/handleCheckin.go
	both modified:   internal/pkg/api/handleCheckin_test.go

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

• #7802 [9.4](backport #7794) refactor: clone ParsedPolicy in processPolicy to prevent shared-state races has been created for branch 9.4 but encountered conflicts

Cherry-pick of c907276 has failed:

On branch mergify/bp/9.4/pr-7794
Your branch is up to date with 'origin/9.4'.

You are currently cherry-picking commit c907276.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   internal/pkg/policy/monitor.go
	modified:   internal/pkg/policy/parsed_policy.go
	modified:   internal/pkg/policy/parsed_policy_test.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   internal/pkg/api/handleCheckin.go
	both modified:   internal/pkg/api/handleCheckin_test.go
	both modified:   internal/pkg/model/ext.go

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

• #7803 [8.19](backport #7794) refactor: clone ParsedPolicy in processPolicy to prevent shared-state races has been created for branch 8.19 but encountered conflicts

Cherry-pick of c907276 has failed:

On branch mergify/bp/8.19/pr-7794
Your branch is up to date with 'origin/8.19'.

You are currently cherry-picking commit c907276.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   internal/pkg/policy/parsed_policy.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   internal/pkg/api/handleCheckin.go
	both modified:   internal/pkg/api/handleCheckin_test.go
	both modified:   internal/pkg/model/ext.go
	both modified:   internal/pkg/policy/monitor.go
	both modified:   internal/pkg/policy/parsed_policy_test.go

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