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
nfrisby,
jasagredo,
fraser-iohk,
dnadales,
geo2a and
jorisdral
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 |
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
2 tasks
amesgen
force-pushed
the
amesgen/v2-ledgerseq-close
branch
from
0dcd9c5 to
981971e
Compare
jasagredo
approved these changes
May 21, 2025
amesgen
force-pushed
the
amesgen/v2-ledgerseq-close
branch
from
981971e to
0c5b137
Compare
jasagredo
approved these changes
Jun 5, 2025
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
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).