Persist datastore changes before caching#933
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Please break this into at least 3 commits, one per fix/method touched.
benthecarman
force-pushed
the
data-store-insert-or-update
branch
from
c4ad5e9 to
36da69d
Compare
Build the updated object separately and persist it before replacing the cached entry, so failed writes leave memory aligned with storage. This finding was discovered by Project Loupe AI-Assisted-By: OpenAI Codex
Apply updates to a cloned object and only replace the cached entry after the backing store write succeeds. This finding was discovered by Project Loupe AI-Assisted-By: OpenAI Codex
Check for the object first, remove it from the backing store, and only then delete it from the in-memory map. This finding was discovered by Project Loupe AI-Assisted-By: OpenAI Codex
Add regression coverage for insert's existing persist-before-cache behavior and update datastore reader comments to match the completed write ordering. This finding was discovered by Project Loupe AI-Assisted-By: OpenAI Codex
benthecarman
force-pushed
the
data-store-insert-or-update
branch
from
36da69d to
0d2d71a
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks! I think at the time we actually preferred to avoid all the re-allocations as we expected to panic on persistence failures anyways. I think that might still be the case, but this PR is also fine.
Might need a rebase after #943 landed. This has me realizing that #943 maybe should have been less broad, i.e., maybe it should have only switched to PaginatedKVStore where we actually require pagination.
3 participants
Keep the in-memory datastore aligned with the persistent store when writes fail. Updates, insert-or-update calls, and removals now leave the cached state unchanged unless the backing write succeeds.
Add regression coverage for failed datastore persistence across insert, update, insert-or-update, and remove paths.
Issue found by Project Loupe