Skip to content

Fix two potential race conditions in project-related code#171

Open
adamwg wants to merge 2 commits into
crossplane:mainfrom
adamwg:awg/racy
Open

Fix two potential race conditions in project-related code#171
adamwg wants to merge 2 commits into
crossplane:mainfrom
adamwg:awg/racy

Conversation

@adamwg

@adamwg adamwg commented Jun 29, 2026

Copy link
Copy Markdown
Member

Description of your changes

These two potential race conditions in project-related code were reported in up but are present here as well and could cause crashes due to concurrent map accesses:

  1. Go schema generation calls some OpenAPI code from Kubernetes libraries that uses the global scheme.Scheme. Our dev control plane clients also used scheme.Scheme (the default), so if we called AddToScheme at an inopportune time it could race. Fix this by using a fresh scheme for the dev control plane client.
  2. We generate schemas for all languages in parallel, which could result in concurrent map writes in the schema generation code. Fix this by adding a mutex to protect the map of schemas.

I have:

adamwg added 2 commits June 29, 2026 13:24
Avoid races with other code that uses the global `scheme.Scheme` by creating a
fresh scheme for the control plane client. This allows us to safely create dev
control planes concurrently with other operations.

Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
We generate schemas for all languages concurrently and previously wrote to the
`schemas` map without coordination.

Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Two independent bug fixes: EnsureLocalDevControlPlane now constructs its controller-runtime client with an explicit runtime.Scheme populated via a new newScheme() helper that registers client-go Kubernetes types. Separately, Manager.Generate adds a local mutex to guard concurrent goroutine writes to the shared schemas map.

Changes

Explicit Kubernetes scheme for controller-runtime client

Layer / File(s) Summary
newScheme helper and client wiring
internal/project/controlplane/controlplane.go
Adds imports for k8s.io/apimachinery/pkg/runtime and clientgoscheme, introduces newScheme() to initialize and register client-go types, and passes the result to client.New via client.Options{Scheme: newScheme()}.

Concurrent map write protection in schema generation

Layer / File(s) Summary
Mutex-guarded schemas map writes
internal/schemas/manager/manager.go
Declares a local schemasMu sync.Mutex in Generate and locks it around each schemas[gen.Language()] = schemaFS assignment inside goroutines.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the two race-condition fixes in project-related code.
Description check ✅ Passed The description directly explains both changes and their motivation, matching the pull request content.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Breaking Changes ✅ Passed Actual diff changes only internal/schemas/manager/manager.go; no files under apis/** or cmd/** were touched, so this check doesn't apply.
Feature Gate Requirement ✅ Passed PASS: The changes are internal race fixes (fresh scheme, mutex) and don't add new apis/** features or experimental behavior.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/project/controlplane/controlplane.go (1)

341-345: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Return the AddToScheme error from newScheme internal/project/controlplane/controlplane.go:341-345

clientgoscheme.AddToScheme can fail, and this helper currently drops that error. Could newScheme return (*runtime.Scheme, error) and wrap the failure close to the source so the control plane setup fails with clearer context?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/project/controlplane/controlplane.go` around lines 341 - 345,
newScheme currently discards the error from clientgoscheme.AddToScheme, so
update it to return (*runtime.Scheme, error) and propagate any failure instead
of ignoring it. In the newScheme helper, check the AddToScheme result and wrap
the error with clear context close to the source, then update the control plane
setup call sites to handle the returned error from newScheme so initialization
fails cleanly.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/project/controlplane/controlplane.go`:
- Around line 341-345: newScheme currently discards the error from
clientgoscheme.AddToScheme, so update it to return (*runtime.Scheme, error) and
propagate any failure instead of ignoring it. In the newScheme helper, check the
AddToScheme result and wrap the error with clear context close to the source,
then update the control plane setup call sites to handle the returned error from
newScheme so initialization fails cleanly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d7911130-09dc-45a1-85ee-c44d661c3521

📥 Commits

Reviewing files that changed from the base of the PR and between 86f5f7a and 7508aa8.

📒 Files selected for processing (2)
  • internal/project/controlplane/controlplane.go
  • internal/schemas/manager/manager.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant