fix(keyval): make FilesystemStore._set_raw durable and leak-free#188
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #188 +/- ##
==========================================
+ Coverage 87.61% 87.64% +0.02%
==========================================
Files 57 57
Lines 2932 2938 +6
==========================================
+ Hits 2569 2575 +6
Misses 363 363 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request enhances the durability of the filesystem key-value store by implementing flush and fsync during writes and ensuring temporary files are deleted upon failure. A new test case validates the cleanup logic. Feedback suggests using a finally block instead of except Exception to more robustly handle BaseException types, such as KeyboardInterrupt, preventing potential file leaks.
tomasvotava
added a commit
that referenced
this pull request
May 19, 2026
…ally Addresses gemini-code-assist review on PR #188: except Exception missed BaseException (e.g. KeyboardInterrupt during write), which would still strand the temp file. try/finally always cleans up; on success os.replace already moved the file so unlink(missing_ok=True) is a no-op. Keeps the explicit tmp_path: str | None annotation (mypy-strict rejects the unannotated form the bot suggested). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to PR #185 (gemini-code-assist review on the sibling FileQueue): the explicitly-mirrored FilesystemStore._set_raw had the same two gaps — no flush()/fsync() before the atomic rename (a crash just after os.replace could leave an empty/truncated value file) and, with delete=False, a write failure stranded the temp file in root_path. Add flush()+fsync() for crash durability and remove the temp file if anything before os.replace fails. (The third FileQueue gap, text encoding, does not apply here — this writer is binary.) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ally Addresses gemini-code-assist review on PR #188: except Exception missed BaseException (e.g. KeyboardInterrupt during write), which would still strand the temp file. try/finally always cleans up; on success os.replace already moved the file so unlink(missing_ok=True) is a no-op. Keeps the explicit tmp_path: str | None annotation (mypy-strict rejects the unannotated form the bot suggested). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
6decca5 to
96ac1a8
Compare
tomasvotava
added a commit
that referenced
this pull request
May 19, 2026
…try/finally Follow-up to the gemini-code-assist review on PR #188 (same atomic-write pattern, applied to #188's keyval sibling). PR #185 had already merged with the except Exception form, which misses BaseException (e.g. KeyboardInterrupt mid-flush) and leaks the temp file beside the queue file. try/finally always cleans up; the success path is a missing_ok no-op since os.replace already renamed it. Keeps FileQueue and FilesystemStore identical. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tomasvotava
added a commit
that referenced
this pull request
May 19, 2026
…try/finally Follow-up to the gemini-code-assist review on PR #188 (same atomic-write pattern, applied to #188's keyval sibling). PR #185 had already merged with the except Exception form, which misses BaseException (e.g. KeyboardInterrupt mid-flush) and leaks the temp file beside the queue file. try/finally always cleans up; the success path is a missing_ok no-op since os.replace already renamed it. Keeps FileQueue and FilesystemStore identical. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tomasvotava
added a commit
that referenced
this pull request
May 19, 2026
…ss path Follow-up to merged #188 / sibling of open #189. The gemini-code-assist review on #189 noted the try/finally cleanup does an unnecessary unlink syscall on every successful write; #188 had already merged with the plain try/finally, so master's FilesystemStore._set_raw needs the same tmp_path=None-after-os.replace optimization to stay identical to FileQueue (#189). Cleanup still runs when an exception occurs before/at the rename. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tomasvotava
added a commit
that referenced
this pull request
May 19, 2026
…ss path Follow-up to merged #188 / sibling of open #189. The gemini-code-assist review on #189 noted the try/finally cleanup does an unnecessary unlink syscall on every successful write; #188 had already merged with the plain try/finally, so master's FilesystemStore._set_raw needs the same tmp_path=None-after-os.replace optimization to stay identical to FileQueue (#189). Cleanup still runs when an exception occurs before/at the rename. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up to #185.
FilesystemStore._set_rawis the atomic-write pattern that #158/#185 was explicitly told to mirror. The gemini-code-assist review on #185 identified durability/leak gaps in the FileQueue copy; the keyval sibling had the same two:flush()/fsync()beforeos.replace— a crash right after the rename could leave an empty/truncated value file. → nowflush()+os.fsync().delete=Falsewith no cleanup — a write failure stranded the temp file inroot_path. → now removed viaPath(...).unlink(missing_ok=True)on error, then re-raise.The third FileQueue gap (text encoding) does not apply — this writer is binary (
"wb").Out of scope:
LocalFileCache's write path has separate, larger safety issues already tracked in #156 — deliberately not folded in here.Tests: new
test_set_cleans_up_temp_file_on_error+ existing suite (19 passed). ruff / ruff-format / mypy-strict / import-linter all green.🤖 Generated with Claude Code