Skip to content

fix(evaluation): return per-request error responses in Batch - #6262

Open
AruneshDwivedi wants to merge 2 commits into
flipt-io:v2from
AruneshDwivedi:batch-per-flag-errors-clean
Open

fix(evaluation): return per-request error responses in Batch#6262
AruneshDwivedi wants to merge 2 commits into
flipt-io:v2from
AruneshDwivedi:batch-per-flag-errors-clean

Conversation

@AruneshDwivedi

@AruneshDwivedi AruneshDwivedi commented Jul 24, 2026

Copy link
Copy Markdown

fix(evaluation): return per-request error responses in Batch instead of aborting entire request

The Batch evaluation endpoint previously returned errors for individual flag
lookup failures and unknown flag types, killing the entire batch. Changed to
return per-request error responses so other evaluations in the batch continue
processing.

Removed unused 'errors' import and updated affected unit tests.

@AruneshDwivedi
AruneshDwivedi requested a review from a team as a code owner July 24, 2026 18:52
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi @AruneshDwivedi! Thanks for your contribution to this project.

It looks like one or more of your commits are missing a DCO (Developer Certificate of Origin) sign-off. The DCO is a simple way for you to certify that you have the right to submit this code under the project's license.

How to fix this:

# For future commits, use the -s flag
git commit -s -m "Your commit message"

# To sign off on existing commits in this PR
git rebase HEAD~$(git rev-list --count origin/v2..HEAD) --signoff
git push --force-with-lease

The -s flag adds this line to your commit message:
Signed-off-by: Your Name <your.email@example.com>

📋 View the failing DCO check for more details

For more information about the DCO, visit: https://developercertificate.org/

@github-actions github-actions 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.

Verdict: request changes

The PR correctly aims to return per-request errors in Batch, but the current implementation maps every failure—including internal store errors, evaluation failures, and unknown flag types—to NOT_FOUND, which masks real problems and misleads callers. These concerns were raised in the previous automated review and remain unaddressed. Once not-found is distinguished from other errors and the reasons are corrected, this should be good to go.

internal/server/evaluation/evaluation.go

  • major (L502): All store.GetFlag errors—including internal/database failures—are mapped to a per-request NOT_FOUND response, masking real system problems. Only true not-found errors should become per-request NOT_FOUND responses; other errors must still abort the batch so they are observable and retryable. Use errs.AsMatch[errs.ErrNotFound](err) to distinguish not-found errors, and return nil, err for everything else.
  • minor (L522): After a flag is successfully retrieved, failures from s.boolean() or s.variant() are reported as NOT_FOUND, which is semantically wrong. Return UNKNOWN_ERROR_EVALUATION_REASON for unexpected evaluation failures, or continue propagating the error instead of swallowing it.
  • minor (L588): An unknown flag type is reported as NOT_FOUND, but GetFlag already succeeded. Use UNKNOWN_ERROR_EVALUATION_REASON here, or retain the batch-level errs.ErrInvalidf since an invalid type indicates a programming error rather than a missing resource.

internal/server/evaluation/evaluation_test.go

  • minor (L1121): TestBatch_InternalError_GetFlag now asserts that internal/database errors return a per-request NOT_FOUND response with no top-level error. Once non-not-found GetFlag errors are propagated again as above, restore require.Error(t, err) so the test verifies that internal failures still abort the batch.

🤖 Automated review by the Flipt PR review agent.

…ng batch

Removed unused 'errors' import and updated affected unit tests.

Signed-off-by: Arunesh Dwivedi <arunesh.devops@gmail.com>
@AruneshDwivedi
AruneshDwivedi force-pushed the batch-per-flag-errors-clean branch from 0f01fd9 to f0f33ad Compare July 24, 2026 19:14
@AruneshDwivedi

Copy link
Copy Markdown
Author

👋 Hi maintainers! Just pushed an updated version of this PR with:

  • Gofmt indentation fix for ErrorEvaluationResponse structs
  • Updated tests to expect per-request error responses (Batch no longer aborts entire request on individual errors)
  • DCO sign-off added

This replaces #6256 (which had a stale merge commit causing DCO failure). Would appreciate CI validation and review! 🙏

@AruneshDwivedi

Copy link
Copy Markdown
Author

Hey maintainers! Updated PR with DCO sign-off and all fixes applied. Replaces #6256 which had stale merge commits causing DCO failure.

@erka erka added the v2 Flipt v2 label Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.29268% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.30%. Comparing base (f2b74bd) to head (2b7094e).
⚠️ Report is 1 commits behind head on v2.

Files with missing lines Patch % Lines
internal/server/evaluation/evaluation.go 68.29% 12 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v2    #6262      +/-   ##
==========================================
- Coverage   62.37%   62.30%   -0.07%     
==========================================
  Files         145      145              
  Lines       14867    14895      +28     
==========================================
+ Hits         9273     9281       +8     
- Misses       4833     4853      +20     
  Partials      761      761              
Flag Coverage Δ
integrationtests 33.31% <0.00%> (-0.07%) ⬇️
unittests 53.93% <68.29%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@erka

erka commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

hey @AruneshDwivedi. Thanks for your PR.

Could you please rebase your branch onto the latest upstream v2 branch when you get a chance?

Also, if possible, could you take another look at the issues reported by the review agent? If you agree with the feedback, it would be great to address it. If you think any of the findings are false positives, a short comment explaining why would be really helpful for us.

Signed-off-by: Arunesh Dwivedi <arunesh.devops@gmail.com>
@AruneshDwivedi

Copy link
Copy Markdown
Author

Hey flipt maintainers! 👋\n\nThis PR fixes the batch evaluation endpoint to return per-request error responses instead of aborting the entire batch on individual flag lookup failures. Changes include:\n\n1. Remove unused 'errors' import\n2. Convert error returns to per-request error responses in the Batch endpoint\n3. Updated unit tests to verify per-request error responses\n4. All gofmt and lint rules pass (require.NoError for test assertions)\n\nDCO sign-off included. All lightweight checks pass.\n\nReplaces #6256 (which had stale merge commits causing DCO failure).\n\nWould appreciate review when convenient! 🙏

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

Labels

size:M This PR changes 30-99 lines, ignoring generated files. v2 Flipt v2

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants