Skip to content

fix: readyz never true of multiple gRPC syncs are used - #1985

Merged
toddbaert merged 1 commit into
open-feature:mainfrom
anxkhn:fix/grpc-sync-per-instance-readiness
Jul 24, 2026
Merged

fix: readyz never true of multiple gRPC syncs are used#1985
toddbaert merged 1 commit into
open-feature:mainfrom
anxkhn:fix/grpc-sync-per-instance-readiness

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 5, 2026

Copy link
Copy Markdown
Contributor
## This PR

The gRPC sync guards its one-shot readiness with a package-level `sync.Once`:

```go
var once msync.Once
// ...
func (g *Sync) handleFlagSync(...) error {
    once.Do(func() {
        g.ready = true
    })
    // ...
}

Because that Once is shared across every grpc.Sync in the process, only the
first instance whose stream connects runs the closure and sets its own ready
flag. Every other grpc.Sync instance stays not-ready forever, so its
IsReady() keeps returning false.

flagd builds a distinct grpc.Sync per configured grpc source
(SyncBuilder.newGRPC in core/pkg/sync/builder/syncbuilder.go), and the
runtime only reports ready once every sync is ready (Runtime.isReady in
flagd/pkg/runtime/runtime.go returns false if any Syncs[].IsReady() is
false). The net effect is that any flagd configured with two or more grpc://
sync sources never passes /readyz, even after all of its gRPC streams are
healthy, so the pod stays NotReady.

The fix

Set g.ready = true directly in handleFlagSync and drop the package-level
once (and its now-unused sync import alias). The assignment is idempotent,
so this is a no-op for the single-source case and makes readiness per instance
for the multi-source case. This also matches the other sync implementations:
the blob, file, and http syncs already set a plain ready bool directly, and
kubernetes tracks it per instance with an atomic.Bool. The package-level
Once was the outlier.

The write still happens-before the update pushed onto the dataSync channel,
so no new concurrent read path is introduced and the change stays race-clean.

Tests

Adds Test_MultipleSyncsBecomeReady, which starts two independent grpc.Sync
instances (each backed by its own bufconn server), drives each one, and asserts
both report IsReady() == true. The test synchronizes by receiving a payload
from the sync channel before reading IsReady(), so it establishes a
happens-before edge and is race-free by construction rather than by timing.

Before this change the test fails on the second instance
("sync 1 should be ready after its stream connects"); after it, both pass.
Readiness was previously untested in this file, so this also closes that
coverage gap.

Verified with:

  • go build ./...
  • go test -race -covermode=atomic -cover -short ./pkg/sync/grpc/ (from core/)
  • golangci-lint run ./pkg/sync/grpc/...

### Notes on the clean body
- No upstream issue exists for this bug, so the PR does not claim to close one.
  Before opening, do a final search for a matching readiness/`/readyz` issue; if
  none, file a short issue describing the multi-grpc-source NotReady symptom and
  link it, or open the PR as a standalone fix (both are acceptable here).
- Diff is 2 files, +60/-6 (source: -6, test: +59). Surgical.

The gRPC sync guarded its one-shot readiness with a package-level
sync.Once. Because that Once is shared across every grpc.Sync in the
process, only the first instance whose stream connects ran the closure
and set its own ready flag; all other instances stayed not-ready
forever, and IsReady() kept returning false for them.

flagd builds a distinct grpc.Sync per configured grpc source and the
runtime only reports ready once every sync is ready, so any flagd with
two or more grpc sources never passed /readyz even after all streams
were healthy.

Set g.ready directly in handleFlagSync (the assignment is idempotent),
which makes readiness per instance, matching the other sync
implementations. Add a regression test that starts two independent
grpc.Sync instances and asserts both become ready.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
@anxkhn
anxkhn requested review from a team as code owners July 5, 2026 09:13
@netlify

netlify Bot commented Jul 5, 2026

Copy link
Copy Markdown

Deploy Preview for polite-licorice-3db33c canceled.

Name Link
🔨 Latest commit 20e0398
🔍 Latest deploy log https://app.netlify.com/projects/polite-licorice-3db33c/deploys/6a4a205760d76f00086e3977

@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 5, 2026
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The sync.Once-based readiness guard in the gRPC sync handler is removed, so g.ready is now set unconditionally each time the stream handler runs instead of only once. A new test validates that readiness is tracked independently across multiple Sync instances.

Changes

Readiness Flag Fix and Test

Layer / File(s) Summary
Remove shared Once guard for readiness
core/pkg/sync/grpc/grpc_sync.go
Removes the sync import alias and package-level once variable, and changes handleFlagSync to set g.ready = true unconditionally instead of via once.Do(...).
Test multi-instance readiness
core/pkg/sync/grpc/grpc_sync_test.go
Adds Test_MultipleSyncsBecomeReady with a newReadySync helper creating isolated bufconn-based Sync instances, and asserts both become ready independently after streaming payloads.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
    participant Test as Test_MultipleSyncsBecomeReady
    participant Sync1 as Sync Instance 1
    participant Sync2 as Sync Instance 2
    participant Server as bufconn gRPC Server

    Test->>Sync1: Sync(ctx1, dataSync)
    Test->>Sync2: Sync(ctx2, dataSync)
    Sync1->>Server: stream request (handleFlagSync)
    Server-->>Sync1: SyncFlags payload
    Sync1->>Sync1: set ready = true
    Sync2->>Server: stream request (handleFlagSync)
    Server-->>Sync2: SyncFlags payload
    Sync2->>Sync2: set ready = true
    Test->>Sync1: IsReady()
    Test->>Sync2: IsReady()
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check ✅ Passed The title clearly summarizes the fix for multi-gRPC-source readiness being incorrectly shared.
Description check ✅ Passed The description directly explains the same gRPC readiness bug, fix, and added regression test.

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.

@sonarqubecloud

sonarqubecloud Bot commented Jul 5, 2026

Copy link
Copy Markdown

@toddbaert toddbaert changed the title fix(sync/grpc): track readiness per Sync instance fix: track readiness per Sync instance Jul 24, 2026
@toddbaert toddbaert changed the title fix: track readiness per Sync instance fix: readyz never true of multiple gRPC syncs are used Jul 24, 2026
@toddbaert
toddbaert merged commit 29833e8 into open-feature:main Jul 24, 2026
19 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 24, 2026
toddbaert pushed a commit that referenced this pull request Jul 27, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>flagd: 0.16.1</summary>

##
[0.16.1](flagd/v0.16.0...flagd/v0.16.1)
(2026-07-27)


### 🐛 Bug Fixes

* add server ReadTimeouts, update security deps
([#1980](#1980))
([0e1e8b3](0e1e8b3))
* handle invalid selector errors
([#1976](#1976))
([409a62e](409a62e))
* **security:** update module google.golang.org/grpc to v1.82.1
[security] ([#2005](#2005))
([4281adf](4281adf))
* **security:** update vulnerability-updates [security]
([#1984](#1984))
([2a5a6b6](2a5a6b6))


### ✨ New Features

* **sync:** configurable gRPC keepalive enforcement on sync server
([#1999](#1999))
([19e57b9](19e57b9))
</details>

<details><summary>flagd-proxy: 0.9.7</summary>

##
[0.9.7](flagd-proxy/v0.9.6...flagd-proxy/v0.9.7)
(2026-07-27)


### 🐛 Bug Fixes

* add server ReadTimeouts, update security deps
([#1980](#1980))
([0e1e8b3](0e1e8b3))
* **security:** update module google.golang.org/grpc to v1.82.1
[security] ([#2005](#2005))
([4281adf](4281adf))
* **security:** update vulnerability-updates [security]
([#1984](#1984))
([2a5a6b6](2a5a6b6))
</details>

<details><summary>core: 0.16.1</summary>

##
[0.16.1](core/v0.16.0...core/v0.16.1)
(2026-07-27)


### 🐛 Bug Fixes

* **certreloader:** re-check reload condition under write lock
([#1994](#1994))
([86489da](86489da))
* handle invalid selector errors
([#1976](#1976))
([409a62e](409a62e))
* readyz never true of multiple gRPC syncs are used
([#1985](#1985))
([29833e8](29833e8))
* **security:** update module google.golang.org/grpc to v1.82.1
[security] ([#2005](#2005))
([4281adf](4281adf))
* **security:** update vulnerability-updates [security]
([#1984](#1984))
([2a5a6b6](2a5a6b6))
* **sync:** avoid send on closed channel in fileinfo watcher Close
([#1992](#1992))
([a7339ea](a7339ea))
* **sync:** handle non-string YAML map keys in config parsing
([#1990](#1990))
([3c72e6f](3c72e6f))
* **sync:** re-establish file watch after delete and restore
([#2001](#2001))
([d434adf](d434adf))
* **sync:** synchronize readiness state
([#2006](#2006))
([317a7e0](317a7e0))


### ✨ New Features

* improve ETag and body hash support to blob/http sync
([#1991](#1991))
([1e210d4](1e210d4))


### 🧹 Chore

* fix flaky test
([7ff137d](7ff137d))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants