fix: resolve missing dependencies in incremental feature flag evaluation - #2135
Conversation
Signed-off-by: Alessandro Yuichi Okimoto <yuichijpn@gmail.com>
Signed-off-by: Alessandro Yuichi Okimoto <yuichijpn@gmail.com>
Signed-off-by: Alessandro Yuichi Okimoto <yuichijpn@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical issue where incremental feature flag evaluation failed with "feature not found" errors when features had multiple dependencies. The root cause was that the dependency resolution logic found direct dependents but missed their transitive dependencies.
- Fixed transitive dependency resolution by implementing complete closure in both Go and TypeScript
- Enhanced API layer to pass all features instead of filtering out archived ones during evaluation
- Added comprehensive test coverage for multi-dependency scenarios and edge cases
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/feature/domain/feature.go | Implements iterative transitive closure for complete dependency resolution and removes debug logging |
| pkg/api/api/api_grpc.go | Removes archived feature filtering and adds enhanced error logging for debugging dependency issues |
| evaluation/typescript/src/evaluation.ts | Mirrors Go implementation with transitive closure and fixes archived feature evaluation order |
| evaluation/typescript/src/tests/evaluator/get_eval_features_test.ts | Updates test expectations and improves assertion logic for dependency resolution |
| evaluation/go/evaluation_test.go | Adds comprehensive test coverage for multiple dependency scenarios and edge cases |
| evaluation/go/evaluation.go | Fixes evaluation order to set flag variations for all features before filtering archived ones |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…essing Signed-off-by: Alessandro Yuichi Okimoto <yuichijpn@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| spanGetFeatures.End() | ||
| features := f.([]*featureproto.Feature) | ||
| activeFeatures := s.filterOutArchivedFeatures(features) |
There was a problem hiding this comment.
We can't filter out all archived flags because we must know what has been archived in the last 30 days, so in the SDK response, the client SDK can know what flag can be deleted from the local cache.
| // Check if the dependency exists in the all map | ||
| if dep, ok := all[fid]; ok { | ||
| if dfs(dep) { | ||
| evals[f.Id] = f | ||
| return true | ||
| } |
There was a problem hiding this comment.
This logic prevents a panic error in case the flag is missing for some edge case.
| // Step 2: Ensure complete transitive closure | ||
| // The DFS above finds dependents (who depends on targets), but misses dependencies of those dependents. | ||
| // Example: If target C → dependent A → dependency D, we found A but missed D. | ||
| // Efficiently process only newly discovered features in each iteration. | ||
| processed := make(map[string]struct{}) | ||
| queue := make([]*feature.Feature, 0, len(evals)) | ||
| for _, f := range evals { | ||
| queue = append(queue, f) | ||
| } | ||
|
|
||
| const maxIterations = 100 // Prevent infinite loops in case of circular dependencies | ||
| iteration := 0 | ||
| for len(queue) > 0 && iteration < maxIterations { | ||
| iteration++ | ||
| nextQueue := make([]*feature.Feature, 0) | ||
| for _, f := range queue { | ||
| if _, ok := processed[f.Id]; ok { | ||
| continue | ||
| } | ||
| processed[f.Id] = struct{}{} | ||
|
|
||
| // Find dependencies of f | ||
| dmn := &Feature{Feature: f} | ||
| for _, depID := range dmn.FeatureIDsDependsOn() { | ||
| if dep, ok := all[depID]; ok { | ||
| if _, exists := evals[depID]; !exists { | ||
| evals[depID] = dep | ||
| nextQueue = append(nextQueue, dep) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| queue = nextQueue | ||
| } |
There was a problem hiding this comment.
Here is the fix for our error.
I will keep adding the dependent flags until I add all.
Fix #2136
Problem
Incremental evaluation failed with "feature not found" errors when features had multiple dependencies (2+ prerequisites or FEATURE_FLAG rules). Single dependencies worked fine, which is why this bug went undetected.
Root Cause: The
GetFeaturesDependsOnTargetsfunction found direct dependents but missed their transitive dependencies. For example:Solution
Implemented complete transitive closure in
GetFeaturesDependsOnTargets:Changes
pkg/feature/domain/feature.go- added iterative dependency resolutionevaluation/typescript/src/evaluation.ts- same algorithm for Node.js SDK consistencyImpact