Skip to content

fix: Key registry proto caches on the proto, not its id() - #7085

Open
jiwidi wants to merge 3 commits into
feast-dev:masterfrom
jiwidi:fix/registry-cache-proto-identity
Open

fix: Key registry proto caches on the proto, not its id()#7085
jiwidi wants to merge 3 commits into
feast-dev:masterfrom
jiwidi:fix/registry-cache-proto-identity

Conversation

@jiwidi

@jiwidi jiwidi commented Oct 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

proto_registry_utils caches its results by id(registry_proto) and version_id. SqlRegistry
leaves version_id empty. After a registry proto is freed, a new proto can get the same id(),
and the cache then returns objects from the old registry. In our case write_to_online_store
validated a vector against a feature view from an earlier FeatureStore and rejected valid rows.

The caches now keep a reference to the proto and compare it by identity. The version_id and
project keys are unchanged.

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

  • Unit: two live registry protos that share one id() (patched in the module) each get their own
    value from all three cache decorators. The test fails on the old code on every run.

The existing registry cache tests pass unchanged.

Misc

Found while running a streaming writer against the Milvus online store; independent of the Milvus
online store PRs.

@jiwidi
fix: Key registry proto caches on the proto, not its id()
97ef1fa 
The proto_registry_utils caches keyed on id(registry_proto) and version_id.
SqlRegistry leaves version_id empty. After a registry proto is freed, a new
proto can get the same id, and the cache then returns objects from the old
registry. For example, write_to_online_store validated a vector against a
feature view from an earlier store and rejected valid rows.

The caches now keep the proto and compare it by identity.

Signed-off-by: jiwidi <[email protected]>
@jiwidi
jiwidi requested a review from a team as a code owner October 28, 2026 11:05
@codecov-commenter

codecov-commenter commented Oct 28, 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

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 51.00%. Comparing base (1e5e90c) to head (f7d5168).

Files with missing lines Patch % Lines
...ython/feast/infra/registry/proto_registry_utils.py 90.90% 0 Missing and 1 partial ⚠️
❗ 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    #7085      +/-   ##
==========================================
+ Coverage   50.97%   51.00%   +0.02%     
==========================================
  Files         455      455              
  Lines       56994    56996       +2     
  Branches     8321     8321              
==========================================
+ Hits        29055    29072      +17     
+ Misses      25954    25938      -16     
- Partials     1985     1986       +1
Flag Coverage Δ
go-feature-server 30.58% <ø> (+0.20%) ⬆️
python-unit 52.47% <90.90%> (+0.01%) ⬆️
Files with missing lines Coverage Δ
...ython/feast/infra/registry/proto_registry_utils.py 77.38% <90.90%> (+2.18%) ⬆️

... and 2 files with indirect coverage changes

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 1e5e90c...f7d5168. 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.

patelchaitany
patelchaitany approved these changes Oct 28, 2026
View reviewed changes

@patelchaitany patelchaitany left a comment •
edited
Loading

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.

LGTM.
Non-blocking: Make the ID-reuse regression test deterministic and cover all three cache decorators.

@jiwidi
test: Make the registry cache id-reuse test deterministic
f7d5168 
The test depended on CPython giving the second proto the freed address
of the first. It now patches id() in the module so both live protos
share one id, and it covers all three cache decorators.

Signed-off-by: jiwidi <[email protected]>
@jiwidi

jiwidi commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

LGTM. Non-blocking: Make the ID-reuse regression test deterministic and cover all three cache decorators.

Thanks! Pushed a new commit for itt. The test now patches id on the module so both protos share one id while both are alive, no more relying on the allocator reusing the address, and it is parametrized over the three decorators (list_project_metadata, list_entities, get_any_feature_view). Checked it fails on every run against the old code.

This branch has not been deployed

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

Reviewers

1 more reviewer
@patelchaitany patelchaitany patelchaitany approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jiwidi @codecov-commenter @patelchaitany