GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: Stop DynamoDB update_online_store overwriting the item when its read fails by ericdelorefice · Pull Request #7046 · feast-dev/feast

Conversation

@ericdelorefice

@ericdelorefice ericdelorefice commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Problem

DynamoDBOnlineStore.update_online_store and update_online_store_async (the async one is what FeatureStore.update_online_store calls) read the stored item with get_item before applying list_append. A ClientError from that read is caught with except ClientError: pass (dynamodb.py 1010-1011, async 1151-1152). The item is then treated as missing and written with put_item (1032, async 1178), which replaces the whole item with only the values in this call. The call returns normally.

For a list_append feature, the stored history is replaced by just the new elements. When the store method is called directly with only some features, the features left out are dropped as well. (FeatureStore.update_online_store requires every feature column, so from there it is the list history that goes.)

A missing item comes back as a response without "Item", not as an exception, so the handler never covered the not-found case. The update_item branch of the same methods already logs and re-raises (1060-1062, async 1211-1213).

The Devin review on #5916 pointed at this handler in January. This PR adds a reproduction, tests and the fix.

Reproduction

moto's DynamoDB server on localhost with the real clients (aiobotocore for the async path, boto3 for the sync one). Only the read failure is injected: a ProvisionedThroughputExceededException on GetItem. I have not seen this in a production deployment. A realistic cause is a provisioned table whose read capacity is throttled while its write capacity is not, since the two are set separately.

Through FeatureStore.update_online_store on master, with update_expressions={"transactions": "list_append(transactions, :new_val)"}:

stored:              transactions=['tx1', 'tx2', 'tx3']  user_score=110
update (tx4, 120) with one throttled GetItem -> returns normally
stored:              transactions=['tx4']  user_score=120
get_online_features: transactions=[['tx4']]

Calling the sync update_online_store directly with only transactions, and the throttle returned as an HTTP 400 on every GetItem attempt (botocore retried, 3 attempts, then raised):

before: {'transactions': ['tx1', 'tx2', 'tx3'], 'user_score': 100, 'segment': 'gold'}
after:  {'transactions': ['tx4']}

With this change both calls raise ProvisionedThroughputExceededException and the stored item is unchanged.

Change

Both handlers log the error and raise, the same as the update_item branch. The intended behaviour change: a failed read now raises out of update_online_store and FeatureStore.update_online_store instead of writing. Entities earlier in the same call stay written, as they already do when update_item fails. A missing item and a successful read behave as before.

Verification

Five new tests in tests/unit/infra/online_store/test_dynamodb_online_store.py. With the source reverted to master and the tests kept, 2 fail: a failed read, sync (moto) and async (mocked client), must raise and leave the item alone. The 3 that pass are the controls: an update keeps features it does not name (sync), a missing item is created (async), and an existing item is updated in place with the list merged (async). Each direction was run 5 times with the same result.

The DynamoDB online store unit tests go from 47 passed to 52 passed. ruff check and ruff format --check are clean on both files, and mypy is clean on dynamodb.py.

Which issue(s) this PR fixes:

None filed. The reproduction is above.

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

Misc

The read-modify-write is still not atomic when two updates for the same entity run at once. That is a separate question, so I left it alone here.

This PR was written with AI assistance (Claude Code); the tests above were run before opening it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ericdelorefice
ericdelorefice requested a review from a team as a code owner October 19, 2026 11:11
@codecov-commenter

codecov-commenter commented Oct 19, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 51.88%. Comparing base (bf74b10) to head (38e4423).
⚠️ Report is 6 commits behind head on master.
❗ 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    #7046      +/-   ##
==========================================
+ Coverage   51.79%   51.88%   +0.08%     
==========================================
  Files         458      458              
  Lines       57715    57741      +26     
  Branches     8483     8487       +4     
==========================================
+ Hits        29896    29961      +65     
+ Misses      25793    25750      -43     
- Partials     2026     2030       +4     
Flag Coverage Δ *Carryforward flag
go-feature-server 30.58% <ø> (ø) Carriedforward from bf74b10
python-unit 53.39% <100.00%> (+0.09%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
sdk/python/feast/infra/online_stores/dynamodb.py 72.89% <100.00%> (+7.54%) ⬆️

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 2f7f57e...38e4423. 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@haoxu0 haoxu0 added the ok-to-test label Oct 20, 2026 — with ChatGPT Codex Connector
@ericdelorefice
ericdelorefice force-pushed the dynamodb-update-read-errors branch from e244d3c to ae36aa2 Compare October 21, 2026 18:28
@ericdelorefice
ericdelorefice force-pushed the dynamodb-update-read-errors branch from ae36aa2 to 2beac3b Compare October 29, 2026 01:20
shuchu
shuchu approved these changes Nov 4, 2026
View reviewed changes

@shuchu shuchu left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ericdelorefice @ntkathole
fix: Stop DynamoDB update_online_store overwriting the item when its …
38e4423
…read fails

update_online_store and update_online_store_async read the stored item
with get_item before applying list_append / prepend. A ClientError from
that read (for example ProvisionedThroughputExceededException once
botocore's retries are used up) was swallowed, so the item was treated as
missing and put_item replaced it with only the values in the call: the
list history and any feature not in the call were lost, and the call
returned normally.

A missing item is reported by the absence of "Item" in the response, not
by an exception, so the handler never covered the not-found case. Log
and re-raise, as the update_item branch of the same methods already does.

Signed-off-by: Eric Del Orefice <[email protected]>
@ntkathole
ntkathole force-pushed the dynamodb-update-read-errors branch from db6a113 to 38e4423 Compare November 11, 2026 14:40

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

@shuchu shuchu shuchu approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@ericdelorefice @codecov-commenter @shuchu @haoxu0
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.