verifyMigratedDatabase: close opened handles when a later open fails - #2022
Open
kriszyp wants to merge 2 commits into
Open
verifyMigratedDatabase: close opened handles when a later open fails#2022kriszyp wants to merge 2 commits into
kriszyp wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates verifyMigratedDatabase in bin/copyDb.ts to track open database handles in an array, ensuring they are all closed in a finally block even if an error occurs during initialization. The reviewer noted that pushing all individual column family handles (rawDbi) to this array keeps them open concurrently for the entire verification process, which could exceed file descriptor limits. They suggested closing each rawDbi immediately after use while keeping only the main database handles in the tracking array.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. |
kriszyp
added a commit
that referenced
this pull request
Jul 31, 2026
…iew on #2022) Keeps the v5.1 backport identical to main: only rootStore/dbisDb need the leak-safety array; per-table CF handles close immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
July 31, 2026 09:17
kriszyp
requested review from
heskew and
ldt1996
and removed request for
sleekmountaincat
July 31, 2026 12:14
…tial-open failure Track every open in a handles array closed in finally (the copyDbToRocks pattern): a throw opening the second store leaked the first, holding the RocksDB lock on the diagnostic path operators use after a broken migration. Caught by the review bots on the v5.1 backport (#2020) of the change that introduced it (#2014). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/2012-verify-handle-leak
branch
from
July 31, 2026 12:25
67895b2 to
e2ee8c2
Compare
…iew) Only rootStore/dbisDb need the leak-safety array (opened before the loop can fail); per-table CF handles close immediately so a many-table sweep does not hold them all open against fd limits. Co-Authored-By: Claude Fable 5 <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
1 participant
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.
Small follow-up to #2014:
verifyMigratedDatabaseopenedrootStoreanddbisDbbefore itstry, so a throw opening the second store (lock contention, corrupt CF) leaked the first — holding the RocksDB lock on exactly the diagnostic path operators use after a broken migration. Now every open is tracked in a handles array closed infinallywith per-handle error guarding, the same patterncopyDbToRocksuses.Caught by the gemini + claude review bots on the v5.1 backport (#2020, fixed there in a6eb7b9); this ports the identical fix to main. No
patchlabel needed — #2020 already carries it for the v5.1 line.Migration unit tests: 7 passing under LMDB engine locally.
🤖 Generated with Claude Code (Claude Fable 5)