fix: protect lazy workClient init with double-checked lock#79
Conversation
… race When the platform-api pod starts before maestro-server finishes its database-migration init container, NewMaestroGRPCSourceWorkClient fails and leaves workClient nil. Previously this caused every subsequent POST /api/v0/work request to return HTTP 500 for the lifetime of the pod, since the nil check returned a plain error (not maestro.Error) which the work handler mapped to 500. Fix: attempt a lazy re-initialization on the first CreateManifestWork call when workClient is nil. By that point maestro is typically healthy (the e2e tests run ~44 min after provision starts), so the re-init succeeds and the request proceeds normally without needing a pod restart. Fixes nightly-ephemeral CI failure observed on 2026-04-05 (build 2040641173546078208): "should be able to create new manifestwork on each of the registered management clusters" — expected 201, got 500. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Concurrent calls to CreateManifestWork could each pass the nil check and race to create multiple gRPC clients. Serialize the check-then-init sequence with workClientMu so only one goroutine ever calls NewMaestroGRPCSourceWorkClient; the outer unsynchronized nil check keeps the hot path lock-free once the client is initialized. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rrp-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @rrp-bot. Thanks for your PR. I'm waiting for a openshift-online member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary by CodeRabbit
WalkthroughThe changes implement safe lazy initialization for the gRPC work client in the maestro client. When Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/clients/maestro/client.go`:
- Around line 418-424: The lazy-init branch constructs the long-lived gRPC work
client with the per-request ctx, which can bind the stored Client.grpc work
client to a cancellable request context; update the lazy-init call to
grpcsource.NewMaestroGRPCSourceWorkClient to use context.Background() (same as
the startup path) so the created client’s lifetime isn’t tied to a request;
locate the construction in the lazy-init code that passes ctx along with
c.openapiClient, c.grpcOpts, and c.sourceID and replace the ctx argument with
context.Background().
- Around line 412-432: The double-checked locking on the plain interface field
c.workClient is a data race; replace the plain field + workClientMu double-check
with a concurrency-safe pattern: store the work client in an atomic.Value (e.g.,
c.workClientVal) holding the workv1client.WorkV1Interface and use
atomic.Value.Load() for the lock-free fast path and atomic.Value.Store() inside
the locked initialization block that calls
grpcsource.NewMaestroGRPCSourceWorkClient; alternatively, if you accept
single-attempt initialization, replace the whole lazy-init with sync.Once to
call NewMaestroGRPCSourceWorkClient and set c.workClient. Ensure all
reads/writes that reference c.workClient (calls like
ManifestWorks(...).Create(...)) are changed to read from the atomic.Value (or
the sync.Once-populated field) to avoid racy interface accesses while keeping
the existing log/adapter (loggerAdapter) and error-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 97f31425-3a1e-43ed-9cae-66a58ec24631
📒 Files selected for processing (1)
pkg/clients/maestro/client.go
| if c.workClient == nil { | ||
| return nil, fmt.Errorf("gRPC work client not initialized") | ||
| c.workClientMu.Lock() | ||
| if c.workClient == nil { | ||
| c.logger.Info("gRPC work client not initialized at startup, attempting lazy initialization", | ||
| "cluster", clusterName) | ||
| adaptedLogger := &loggerAdapter{logger: c.logger} | ||
| wc, err := grpcsource.NewMaestroGRPCSourceWorkClient( | ||
| ctx, | ||
| adaptedLogger, | ||
| c.openapiClient, | ||
| c.grpcOpts, | ||
| c.sourceID, | ||
| ) | ||
| if err != nil { | ||
| c.workClientMu.Unlock() | ||
| return nil, fmt.Errorf("gRPC work client initialization failed: %w", err) | ||
| } | ||
| c.workClient = wc | ||
| } | ||
| c.workClientMu.Unlock() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no existing concurrent test exercising the DCL path, and inspect the
# return type of NewMaestroGRPCSourceWorkClient to choose between atomic.Value and
# atomic.Pointer[wrapper].
fd -e go . | xargs rg -nP '\bCreateManifestWork\s*\(' -C2
rg -nP --type=go -C3 '\bNewMaestroGRPCSourceWorkClient\s*\('Repository: openshift-online/rosa-regional-platform-api
Length of output: 3273
🏁 Script executed:
# Search for grpcsource.NewMaestroGRPCSourceWorkClient implementation to understand return type and context handling
rg -nP --type=go -B2 -A10 'func NewMaestroGRPCSourceWorkClient'Repository: openshift-online/rosa-regional-platform-api
Length of output: 69
🏁 Script executed:
# Check the workClient field declaration in the Client struct
rg -nP --type=go -B2 -A2 'type Client struct' pkg/clients/maestro/client.go | head -50Repository: openshift-online/rosa-regional-platform-api
Length of output: 213
🏁 Script executed:
# Look for any race detector configuration in tests (go.mod, Makefile, test files)
find . -name 'Makefile' -o -name '*.mk' -o -name 'go.mod' | xargs grep -l 'race' 2>/dev/null || echo "No race config found"
fd -e go pkg/clients/maestro | xargs grep -l 'TestCreateManifestWork\|CreateManifestWork.*test' 2>/dev/null || echo "No unit tests for CreateManifestWork found"Repository: openshift-online/rosa-regional-platform-api
Length of output: 550
🏁 Script executed:
# Get the full Client struct definition
sed -n '76,120p' pkg/clients/maestro/client.goRepository: openshift-online/rosa-regional-platform-api
Length of output: 1463
🏁 Script executed:
# Check imports in client.go to understand where grpcsource comes from
head -20 pkg/clients/maestro/client.goRepository: openshift-online/rosa-regional-platform-api
Length of output: 522
🏁 Script executed:
# Check Makefile for race detector configuration
grep -A3 -B3 'race' ./Makefile | head -20Repository: openshift-online/rosa-regional-platform-api
Length of output: 721
🏁 Script executed:
# Search for any integration or concurrent tests
fd '.*test.*\.go$' pkg/handlers | xargs grep -l 'CreateManifestWork\|t\.Parallel\|sync\.' 2>/dev/null | head -5Repository: openshift-online/rosa-regional-platform-api
Length of output: 150
🏁 Script executed:
# Check work_test.go for concurrent tests and CreateManifestWork usage
cat -n pkg/handlers/work_test.goRepository: openshift-online/rosa-regional-platform-api
Length of output: 19277
🏁 Script executed:
# Check if there are any imports or references to grpcsource return type documentation
rg -C5 'workv1client.WorkV1Interface' pkg/clients/maestro/Repository: openshift-online/rosa-regional-platform-api
Length of output: 756
🏁 Script executed:
# Check the actual context usage in the lazy initialization path vs startup
sed -n '126,135p' pkg/clients/maestro/client.go
sed -n '412,432p' pkg/clients/maestro/client.goRepository: openshift-online/rosa-regional-platform-api
Length of output: 1012
🌐 Web query:
gRPC context lifetime NewClient context.Background vs request context retained
💡 Result:
In gRPC-Go, the ClientConn created by grpc.NewClient(target, opts...) internally uses a long-lived context created from context.Background: cc.ctx, cc.cancel = context.WithCancel(context.Background). This context is cancelled only when ClientConn.Close is called, managing the overall lifetime of the connection, including name resolution, connection establishment, and reconnection attempts. This differs from grpc.DialContext(ctx, target, opts...), where the provided ctx controls only the initial connection attempt (if using WithBlock). The DialContext ctx does not persist or affect the ClientConn's lifetime after creation; RPCs use their own per-call contexts. For individual RPCs (e.g., client.Method(ctx, req)), the provided request ctx always controls that specific call's lifetime, including cancellation and deadlines. These per-request contexts are independent of the ClientConn's internal context and are not "retained" from the NewClient/Dial call. Each RPC must pass an appropriate ctx (typically derived from context.Background or a parent ctx with timeout/deadline). Using context.Background for NewClient (implicitly) or DialContext is correct for long-lived clients, as it never cancels. Per-request contexts should carry cancellation/deadlines appropriate to each call. The ClientConn context ensures graceful shutdown on Close, but RPCs propagate their own ctx cancellation to servers. Official gRPC-Go docs discourage pre-connecting with Dial/WithBlock (anti-pattern), favoring NewClient which starts idle and connects on first RPC.
Citations:
- 1: https://github.com/grpc/grpc-go/blob/master/clientconn.go
- 2: https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md
- 3: https://github.com/grpc/grpc-go/blob/master/examples/route_guide/client/client.go
- 4: https://oneuptime.com/blog/post/2026-01-24-grpc-context-cancellation/view
- 5: https://grpc.io/docs/guides/cancellation/
Double-checked locking on a plain interface field is a data race in Go — go test -race will flag it.
The outer if c.workClient == nil at line 412 reads the interface without holding workClientMu, while another goroutine may concurrently write c.workClient = wc at line 429 under the lock. Per the Go memory model this is undefined behavior:
- The race detector will fail this code when
go test -raceruns (which your Makefile already does for all unit tests). workv1client.WorkV1Interfaceis an interface value (two words: type + data pointer). A racy concurrent read can observe a partially-written state—e.g., a non-nil type word with a nil/garbage data pointer—causing the fast path to proceed and panic with a nil dereference insideManifestWorks(...).Create(...)at line 435.
Use atomic.Value to store the client interface instead of a plain field. This preserves your lock-free fast-path read and retry-on-failure semantics while making reads and writes safe under concurrency. Alternatively, sync.Once works if you can accept no retry (fail once, stay failed).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/clients/maestro/client.go` around lines 412 - 432, The double-checked
locking on the plain interface field c.workClient is a data race; replace the
plain field + workClientMu double-check with a concurrency-safe pattern: store
the work client in an atomic.Value (e.g., c.workClientVal) holding the
workv1client.WorkV1Interface and use atomic.Value.Load() for the lock-free fast
path and atomic.Value.Store() inside the locked initialization block that calls
grpcsource.NewMaestroGRPCSourceWorkClient; alternatively, if you accept
single-attempt initialization, replace the whole lazy-init with sync.Once to
call NewMaestroGRPCSourceWorkClient and set c.workClient. Ensure all
reads/writes that reference c.workClient (calls like
ManifestWorks(...).Create(...)) are changed to read from the atomic.Value (or
the sync.Once-populated field) to avoid racy interface accesses while keeping
the existing log/adapter (loggerAdapter) and error-handling behavior.
| wc, err := grpcsource.NewMaestroGRPCSourceWorkClient( | ||
| ctx, | ||
| adaptedLogger, | ||
| c.openapiClient, | ||
| c.grpcOpts, | ||
| c.sourceID, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the openshift-online/maestro Go SDK, does grpcsource.NewMaestroGRPCSourceWorkClient retain the ctx argument for the lifetime of the returned client (e.g., for background goroutines or watch streams), or is it only used during initial connection setup?
💡 Result:
No definitive source code or documentation found for grpcsource.NewMaestroGRPCSourceWorkClient in the openshift-online/maestro Go SDK despite extensive searches of GitHub, pkg.go.dev, and related OpenShift repositories. The repository https://github.com/openshift-online/maestro exists and contains gRPC APIs as described, but specific function details are not publicly indexed or accessible in search results. In standard Go gRPC clients (e.g., generated from protoc), the ctx argument to NewClient is used only for dialing/connection setup, not retained for the client lifetime. For long-lived clients used in background goroutines or watch streams, a background context (context.Background or long-lived ctx) should be passed during client creation, and per-call contexts used for individual RPCs/streams. The client connection itself is intended to be reused across multiple calls.
Citations:
- 1: https://github.com/openshift-online/maestro
- 2: https://pkg.go.dev/github.com/openshift-online/maestro
🏁 Script executed:
#!/bin/bash
# Locate the vendored/imported source of NewMaestroGRPCSourceWorkClient to inspect ctx usage.
fd -e go . | xargs rg -nP -C5 'func\s+NewMaestroGRPCSourceWorkClient\s*\('Repository: openshift-online/rosa-regional-platform-api
Length of output: 71
🏁 Script executed:
#!/bin/bash
# Find the grpcsource import and related context
cd pkg/clients/maestro
head -50 client.go | grep -A5 -B5 importRepository: openshift-online/rosa-regional-platform-api
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check what happens with wc after it's created (lines 425+)
cd pkg/clients/maestro
sed -n '418,450p' client.goRepository: openshift-online/rosa-regional-platform-api
Length of output: 972
🏁 Script executed:
#!/bin/bash
# Check the startup path at line 128
cd pkg/clients/maestro
sed -n '120,140p' client.goRepository: openshift-online/rosa-regional-platform-api
Length of output: 781
🏁 Script executed:
#!/bin/bash
# Find where grpcsource package is defined or imported from
rg 'grpcsource' pkg/clients/maestro/ -A 2 -B 2Repository: openshift-online/rosa-regional-platform-api
Length of output: 1126
🏁 Script executed:
#!/bin/bash
# Check go.mod to see maestro dependency
cat go.mod | grep maestroRepository: openshift-online/rosa-regional-platform-api
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Check if maestro is vendored
ls -la vendor/ | grep maestroRepository: openshift-online/rosa-regional-platform-api
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Try to find the actual source - search in vendor for grpcsource
find vendor -type f -name "*grpcsource*" 2>/dev/null | head -20Repository: openshift-online/rosa-regional-platform-api
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Search vendor for NewMaestroGRPCSourceWorkClient implementation
rg -i "NewMaestroGRPCSourceWorkClient" vendor/ -A 20 2>/dev/null | head -100Repository: openshift-online/rosa-regional-platform-api
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Check if there are test files that show how the client is used
find . -name "*test.go" -type f | grep maestro | head -10Repository: openshift-online/rosa-regional-platform-api
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about context usage in client.go
cat pkg/clients/maestro/client.go | grep -i -C3 "context\|lifetime\|cancel\|background"Repository: openshift-online/rosa-regional-platform-api
Length of output: 8286
🏁 Script executed:
#!/bin/bash
# Check if workClient is accessed/used multiple times
rg "c\.workClient" pkg/clients/maestro/client.go -nRepository: openshift-online/rosa-regional-platform-api
Length of output: 373
🏁 Script executed:
#!/bin/bash
# Look for concurrent access patterns or request handling
rg -B5 -A5 "workClientMu\|workClient" pkg/clients/maestro/client.goRepository: openshift-online/rosa-regional-platform-api
Length of output: 69
Use context.Background() in the lazy-init path to match the startup path.
The gRPC work client is stored in the Client struct and reused across multiple requests (lines 429, 435). The startup path initializes it with context.Background() (line 128), but the lazy path passes the per-request ctx (line 418). For long-lived clients that are stored and reused, the context passed to the constructor should never be cancelled; using a per-request context here risks binding the client's lifetime to that first request's context. Ensure both paths use context.Background() consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/clients/maestro/client.go` around lines 418 - 424, The lazy-init branch
constructs the long-lived gRPC work client with the per-request ctx, which can
bind the stored Client.grpc work client to a cancellable request context; update
the lazy-init call to grpcsource.NewMaestroGRPCSourceWorkClient to use
context.Background() (same as the startup path) so the created client’s lifetime
isn’t tied to a request; locate the construction in the lazy-init code that
passes ctx along with c.openapiClient, c.grpcOpts, and c.sourceID and replace
the ctx argument with context.Background().
2 participants
Summary
Supersedes #62.
PR #62 introduced lazy re-initialization of
c.workClientinCreateManifestWorkto recover from the maestro startup-ordering race, but the check-then-act sequence was unsynchronized. Under concurrent requests arriving whilec.workClientis still nil, multiple goroutines could each pass the outernilcheck and race to callNewMaestroGRPCSourceWorkClient, leaking connections and overwriting each other's result.This PR keeps the lazy-init approach from #62 and adds a
sync.Mutex(workClientMu) to theClientstruct, implementing a double-checked lock:workClientMu.Lock()+ inner nil check — ensures exactly one goroutine runsNewMaestroGRPCSourceWorkClienteven under concurrent requests.Changes
pkg/clients/maestro/client.go: add"sync"import, addworkClientMu sync.Mutexfield toClient, replace bare nil-guard inCreateManifestWorkwith double-checked lock.Test Plan
POST /api/v0/workreturns 201 even when platform-api pod starts before maestro-server is fully readygo test -race🤖 Generated with Claude Code