feat: scope flag deletion by flagSetId to support per-selector sync

[erenatas]

This PR

When an upstream provider sends per-selector stream messages (each with a flagSetId in metadata),

• store.Update now only deletes stale flags within the same flagSetId(s), preserving flags from other selector.
• Updated ToLogString function since the old ToLogString only handled single-key selectors and returned <none> for compound ones. With the scoped deletion, selectors now have two keys (flagSetId+source), so debug logs would show <none> instead of the actual query — making it impossible to troubleshoot store operations.

Related Issues

This change should fix using flagd with a provider that has multiple selectors

[netlify]

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	8869018
🔍 Latest deploy log	https://app.netlify.com/projects/polite-licorice-3db33c/deploys/69d6611de2e36b00084cbe6c

[gemini-code-assist]

Code Review

This pull request introduces flagSetId-based scoping for flag updates, allowing for more granular control over flag deletions by targeting specific flag sets rather than the entire source. It also improves the selector's log string generation by supporting multiple keys and ensuring deterministic output through sorting. Feedback was provided to refine the scoping logic to correctly handle empty flagSetId strings and to optimize performance by utilizing the existing write transaction for queries instead of creating new read transactions.

[gemini-code-assist]

The current implementation has two areas for improvement:

1. Scoping for Default Selector: The check fsi != "" prevents scoping when the flagSetId is an empty string. Since flagd normalizes empty or missing flagSetIds to a default internal UUID (nilFlagSetId), skipping scoping for "" causes the "default" selector to fall back to source-scoped deletion. This may unintentionally delete flags from other selectors. Removing fsi != "" and relying on ok ensures consistent scoping for all selectors.
2. Transaction Efficiency: Calling s.GetAll inside Update is suboptimal because it initiates new read transactions while a write transaction (txn) is already active. It is more efficient to use the existing txn directly. This also allows for proper error handling during the query phase.

	if fsi, ok := metadata["flagSetId"].(string); ok {
		seenFlagSetIds := map[string]struct{}{fsi: {}}
		for id := range newFlags {
			seenFlagSetIds[id.flagSetId] = struct{}{}
		}
		for fsi := range seenFlagSetIds {
			sel := NewSelector(flagSetIdIndex + "=" + fsi).WithIndex(sourceIndex, source)
			indexId, constraints := sel.ToQuery()
			it, err := txn.Get(flagsTable, indexId, constraints...)
			if err != nil {
				s.logger.Error(fmt.Sprintf("unable to query flags for flagSetId %s: %v", fsi, err))
				continue
			}
			oldFlags = append(oldFlags, s.collect(it)...)
		}
	} else {
		sel := NewSelector(sourceIndex + "=" + source)
		indexId, constraints := sel.ToQuery()
		it, err := txn.Get(flagsTable, indexId, constraints...)
		if err != nil {
			s.logger.Error(fmt.Sprintf("unable to query flags for source %s: %v", source, err))
		} else {
			oldFlags = s.collect(it)
		}
	}

[erenatas]

Flags with an empty-string flagSetId are stored as "", but NewSelector silently rewrites "" to nilFlagSetId, so removing the guard would make the query look for the wrong value and never find those flags.

[erenatas]

Transaction Efficiency: Calling s.GetAll inside Update is suboptimal because it initiates new read transactions while a write transaction (txn) is already active. It is more efficient to use the existing txn directly. This also allows for proper error handling during the query phase.

Updated

[toddbaert]

Hey @erenatas, thanks for this PR! The use case makes sense; I can see how per-flagSetId updates from a single source are very useful for multi-project gRPC backends.

A few thoughts:

I think this is a feature, not a fix. The current source-scoped deletion is working as designed; each update is a complete snapshot for that source. This PR adds support for incremental per-flagSetId updates, which is new behavior. The commit message should reflect that (e.g., feat: not fix:), so I've made that change.

Orphaned flags problem. By scoping deletion to flagSetId, the store moves from a stateless full-snapshot model to a stateful incremental one. This means flags can become orphaned if their flagSetId is renamed, removed, or stops being sent by the upstream; those flags never get deleted. There's currently no mechanism (in the store or the sync proto) to clean these up, short of restarting flagd or sending an update with no flagSetId to trigger a full source-scoped wipe.

Proposal: make this opt-in per source. Rather than implicitly switching deletion strategy based on whether flagSetId is present in metadata, I'm thinking this should be an explicit per-source config option (e.g., on SourceConfig). This way:

• The default behavior is unchanged; no risk for existing users.
• Operators who need per-flagSetId incremental updates opt in explicitly, aware of the tradeoffs, and sending explicit empty flagSetIds for flags they want removed from previous sets
• Sources that include flagSetId in their flag definitions for metadata/selector purposes (but still send full snapshots) aren't silently affected.

[toddbaert]

I really like this idea and I want to fix the problem, but I'm blocking for now because of the concerns in this comment, which also has an alternative proposal.

[erenatas]

I really like this idea and I want to fix the problem, but I'm blocking for now because of the concerns in this comment, which also has an alternative proposal.

I updated the PR to make it opt-in, can you have a look?

[sonarqubecloud]

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.6% Duplication on New Code

See analysis details on SonarQube Cloud