Skip to content

fix: Prevent mutation of cached feature resolution results - #6722

Open
junwoo-choi-rapportlabs wants to merge 1 commit into
feast-dev:masterfrom
junwoo-choi-rapportlabs:fix-context-cache
Open

fix: Prevent mutation of cached feature resolution results#6722
junwoo-choi-rapportlabs wants to merge 1 commit into
feast-dev:masterfrom
junwoo-choi-rapportlabs:fix-context-cache

Conversation

@junwoo-choi-rapportlabs

@junwoo-choi-rapportlabs junwoo-choi-rapportlabs commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown

What this PR does / why we need it:

_get_cached_request_context() caches grouped_refs, including the mutable
requested-feature lists nested inside it.

The Redis online store appends an internal timestamp key to these lists while
preparing HMGET fields. Because cache hits returned the same nested lists,
timestamp keys could accumulate across requests, increasing Redis fields and
response post-processing work over time.

This change copies each requested-feature list at the request preparation
boundary before passing it to an online store. The cached resolution remains
shared, while downstream mutations become request-local.

image

Which issue(s) this PR fixes:

N/A

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

@junwoo-choi-rapportlabs
junwoo-choi-rapportlabs requested a review from a team as a code owner August 10, 2026 07:40
@codecov-commenter

codecov-commenter commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.79%. Comparing base (39d408d) to head (728daea).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6722   +/-   ##
=======================================
  Coverage   46.79%   46.79%           
=======================================
  Files         415      415           
  Lines       50395    50396    +1     
  Branches     7215     7215           
=======================================
+ Hits        23581    23584    +3     
+ Misses      25162    25161    -1     
+ Partials     1652     1651    -1
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 48.12% <100.00%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/utils.py 77.88% <100.00%> (+0.30%) ⬆️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39d408d...728daea. Read the comment docs.

🚀 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.

jyejare
jyejare approved these changes Aug 10, 2026
View reviewed changes

@jyejare jyejare left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes a caching bug where mutations to feature lists were leaking between cached requests. The fix creates defensive copies of grouped_refs to prevent shared mutable state issues. The implementation is correct and includes comprehensive test coverage.

Comment thread sdk/python/feast/utils.py
Comment thread sdk/python/tests/unit/test_feature_resolution_cache.py Outdated
@shuchu

shuchu commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

The DCO warning needs to be handled. please check the warning message.

@junwoo-choi-rapportlabs
fix: Prevent mutation of cached feature resolution results
728daea 
Copy grouped feature lists before passing them to online stores so Redis
timestamp fields do not accumulate in the shared resolution cache.

Signed-off-by: rapportlabs <junwoo@rapportlabs.kr>
@junwoo-choi-rapportlabs

junwoo-choi-rapportlabs commented Aug 11, 2026

Copy link
Copy Markdown
Author

The DCO warning needs to be handled. please check the warning message.

My bad. corrected it.

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

Reviewers

@jyejare jyejare jyejare approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@junwoo-choi-rapportlabs @codecov-commenter @shuchu @jyejare