LedgerDB.V2: make sure to actually close handles #1516
Merged
Conversation
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
amesgen
requested review from
dnadales,
fraser-iohk,
geo2a,
jasagredo,
jorisdral and
nfrisby
as code owners
amesgen
commented
May 19, 2025
Comment on lines
+157
to
+166
| closeDiscarded = do | ||
| closePruned | ||
| -- Do /not/ close the anchor of @toClose@, as that is also the | ||
| -- tip of @olddb'@ which will be used in @newdb@. | ||
| case toClose of | ||
| AS.Empty _ -> pure () | ||
| _ AS.:< closeOld' -> closeLedgerSeq (LedgerSeq closeOld') | ||
| -- Finally, close the anchor of @lseq@ (which is a duplicate of | ||
| -- the head of @olddb'@). | ||
| close $ tables $ AS.anchor lseq |
Member
Author
There was a problem hiding this comment.
I think this is correct, but it is rather non-obvious.
It would be "nicer" if newdb would use the anchor state of lseq instead of the head state of olddb' (but we would need a special purpose AS.join for this as this isn't possible in general when the anchor is "strictly smaller" than the element type). In that case, we could simply write
closeDiscarded = closePruned *> closeLedgerSeq (LedgerSeq closeOld)instead of what I have here.
amesgen
force-pushed
the
amesgen/v2-ledgerseq-close
branch
from
8da1177 to
0dcd9c5
Compare
amesgen
force-pushed
the
amesgen/v2-ledgerseq-close
branch
from
0dcd9c5 to
981971e
Compare
jasagredo
approved these changes
May 21, 2025
Contributor
jasagredo
left a comment
jasagredo
left a comment
There was a problem hiding this comment.
Very nice catch, Alex. The fix looks very reasonable.
amesgen
force-pushed
the
amesgen/v2-ledgerseq-close
branch
from
981971e to
0c5b137
Compare
jasagredo
approved these changes
Jun 5, 2025
Contributor
jasagredo
left a comment
jasagredo
left a comment
There was a problem hiding this comment.
No negative comments. I think we can merge this if you want.
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2/InMemory.hs
Show resolved
Hide resolved
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/LedgerDB/StateMachine.hs
Show resolved
Hide resolved
For tracking whether we are leaking any handles, in a test to be added in a follow-up commit
for testing that we do not leak any, in a follow-up commit
Only temporary, see the linked issue.
amesgen
force-pushed
the
amesgen/v2-ledgerseq-close
branch
from
0c5b137 to
7900088
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Consensus Team Backlog
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 11, 2025
Closes #1551 The commits are intended to be reviewed individually. - The first commit fixes the race condition described in #1551 by using the `ldbOpenHandlesLock` to duplicate the handle when snapshotting. - The second commit reverts 7900088 added in #1516, enabled by the first commit. Without the first commit, this would cause test failures, see the commit description. - The third and fourth commits are purely refactorings to be able to reuse the pattern of "duplicating while holding a read lock". - The fifth commit uses the same pattern for `takeSnapshotNOW`, a testing function, see the commit description. - The last commit adds documentation for `ldbOpenHandlesLock`.
amesgen
added a commit
that referenced
this pull request
Jun 30, 2025
Closes #1515 The first three commits add a regression test by enrichting the existing LedgerDB state machine test, making sure that ``` total number of all opened handles - total number of closed handles = states on the LedgerSeq in the end ``` The fourth commit then fixes the bug, causing the regression test to succeed. Note that this bug fix reveals another bug #1551 (causing the `mock-test` failure for `LeaderSchedule.simple convergence`), which we silence by temporarily making the closing of an in-memory handle a no-op (as it is not *necessary* anyways, the GC will still collect the in-memory ledger state).
amesgen
added a commit
that referenced
this pull request
Jun 30, 2025
Closes #1551 The commits are intended to be reviewed individually. - The first commit fixes the race condition described in #1551 by using the `ldbOpenHandlesLock` to duplicate the handle when snapshotting. - The second commit reverts 7900088 added in #1516, enabled by the first commit. Without the first commit, this would cause test failures, see the commit description. - The third and fourth commits are purely refactorings to be able to reuse the pattern of "duplicating while holding a read lock". - The fifth commit uses the same pattern for `takeSnapshotNOW`, a testing function, see the commit description. - The last commit adds documentation for `ldbOpenHandlesLock`.
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.
Closes #1515
The first three commits add a regression test by enrichting the existing LedgerDB state machine test, making sure that
The fourth commit then fixes the bug, causing the regression test to succeed.
Note that this bug fix reveals another bug #1551 (causing the
mock-testfailure forLeaderSchedule.simple convergence), which we silence by temporarily making the closing of an in-memory handle a no-op (as it is not necessary anyways, the GC will still collect the in-memory ledger state).