#551: Keep fact replacement atomic#554
Conversation
bibonix
left a comment
bibonix
left a comment
There was a problem hiding this comment.
The delete-then-insert sequence in Fbe.delete, Fbe.delete_one, and both branches of Fbe.overwrite now sits inside the fb.txn block, so a raised insert rolls the delete back instead of leaving the factbase with the old fact gone. The three new regression tests inject a raising Factbase::Pre to force the insert to fail and then assert that the original fact and every untouched property survive the call. CI is green on actionlint, copyrights, markdown-lint, pdd, reuse, typos, xcop, yamllint, and the rake jobs on ubuntu-24.04 and macos-15. No inline comments.
|
@GHX5T-SOL merge conflict here |
|
Please fix the merge conflicts so this pull request can be merged. |
morphqdd
left a comment
morphqdd
left a comment
There was a problem hiding this comment.
Reviewed and verified locally. The fix is correct: moving the delete! inside fb.txn makes the delete part of the same transaction as the re-insert, and since Factbase#txn tapes deletions and only applies them to @maps on successful completion (with Factbase::Pre#txn delegating through), a failing re-insert discards the taped delete and the original fact survives.
I reproduced it both ways with a small script using Factbase::Pre that raises on insert:
- master:
Fbe.delete/Fbe.overwrite-> the fact is gone (facts=0) after the insert raises. - this branch: the fact is kept intact (foo=[42], bar=['keep']).
Both overwrite branches, delete, and delete_one are covered, and the new tests genuinely fail on master. LGTM — two minor, non-blocking notes inline.
| raise(Fbe::Error, "There is no #{fid} in the fact, cannot use Fbe.overwrite") if id.nil? | ||
| raise(Fbe::Error, "No facts by #{fid} = #{id}") if fb.query("(eq #{fid} #{id})").delete!.zero? | ||
| fb.txn do |fbt| | ||
| raise(Fbe::Error, "No facts by #{fid} = #{id}") if fbt.query("(eq #{fid} #{id})").delete!.zero? |
There was a problem hiding this comment.
Good that both code paths (the hash branch here and the scalar branch at line 98) got the delete moved inside fb.txn — they're easy to fix only one of. Verified the mechanism: Factbase#txn records deletions on a LazyTaped tape and only applies them to @maps after the block returns, so a raising re-insert never applies the delete (Factbase::Pre#txn delegates, so it holds for wrapped factbases too). One tiny note: the "No facts by ..." guard now runs its delete! inside the txn even on the raise path — harmless (0 rows, rolled back), just slightly more work than the old pre-check.
| def test_keeps_original_fact_when_reinsert_fails | ||
| reject = false | ||
| fb = | ||
| Factbase::Pre.new(Factbase.new) do |_f, _fbt| |
There was a problem hiding this comment.
Nice regression test — I ran it against master and confirmed it fails there (fact count drops to 0), and passes with this change, so it genuinely guards the atomicity. The Factbase::Pre.new(Factbase.new) { |_f, _fbt| raise if reject } + insert boilerplate is repeated verbatim across all three test files; optional, but a small shared helper would keep them from drifting.
|
@morphqdd Thanks for the review! You've earned +13 points for this: +18 as a basis; -5 for very few (2) comments. Your running score is +610; don't forget to check your Zerocracy account too). |
|
@GHX5T-SOL merge conflict here |
5 participants
/claim #551
Closes #551.
Summary
Fbe.overwrite,Fbe.delete, andFbe.delete_oneinside the samefb.txnblock as the replacement insert.Validation
bundle checkbundle exec ruby -Itest test/fbe/test_overwrite.rb -- --no-covbundle exec ruby -Itest test/fbe/test_delete.rb -- --no-covbundle exec ruby -Itest test/fbe/test_delete_one.rb -- --no-covgit diff --checkbundle exec rubocop lib/fbe/overwrite.rb lib/fbe/delete.rb lib/fbe/delete_one.rb test/fbe/test_overwrite.rb test/fbe/test_delete.rb test/fbe/test_delete_one.rbbundle exec rake testbundle exec rubocopbundle exec rakeNo secrets or live-user data used; validation was local and within the repo scope.