[wip] db/state: only last reader deletes merged files#21395
Closed
AskAlexSharov wants to merge 2 commits into
Closed
[wip] db/state: only last reader deletes merged files#21395AskAlexSharov wants to merge 2 commits into
AskAlexSharov wants to merge 2 commits into
Conversation
deleteMergeFile no longer removes files eagerly. It removes the item from dirtyFiles and marks canDelete=true; physical removal is left to the last reader inside RoTx.Close. The merge holds a rotx pinning these files, so it is itself such a reader.
…ks them Since deleteMergeFile no longer unlinks files eagerly, a subsumed file with no live reader (e.g. RemoveOverlapsAfterMerge, or files already non-visible by the final merge step) would be marked canDelete but never closed - leaking the FD and leaving the file on disk. POSIX hides this via unlink-while-open; Windows fails RemoveAll with "Access is denied". cleanAfterMerge now holds a dirty-files rotx (DebugBeginDirtyFilesRo) pinning the subsumed files, so its Close is a guaranteed last reader that unlinks any file no other reader still holds. Add a portable regression test asserting subsumed files are unlinked from disk after RemoveOverlapsAfterMerge.
sudeepdino008
approved these changes
May 26, 2026
Collaborator
Author
|
wrong direction: this func deleting non-visible (garbage) files also Better avoid |
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
2 participants
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.
Summary
deleteMergeFileno longer removes files eagerly. It drops the item fromdirtyFilesand setscanDelete=true; physical removal (closeFilesAndRemove) is left to the last reader insideRoTx.Close.BeginFilesRois lock-free, so eagerly closing a file indeleteMergeFilecan race a reader that loaded a visible generation containing the file but has not yet bumped its refcount.cleanAfterMergenow holds a dirty-files rotx (DebugBeginDirtyFilesRo) pinning the subsumed files, so itsCloseis a guaranteed last reader. Without this, files with no live reader (the final merge step's subsumed files, andRemoveOverlapsAfterMerge) would be markedcanDeletebut never unlinked — a FD/disk leak that POSIX hides (unlink-while-open) but Windows surfaces asRemoveAll: Access is denied.TestCleanAfterMerge_UnlinksSubsumedFiles) that catches the leak on all platforms via on-disk presence.Open items (wip)
SnapshotRepo.DeleteFilesAfterMergestill uses the eagerrefcount==0 → closeFilesAndRemovebranch. It is not broken (still unlinks), but for consistency it should eventually move to the same last-reader model.