Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(db): version field migration never completing #4585

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Mar 16, 2024

I've noticed the fedimint-cli keeps printing over and over:

2024-03-16T21:45:20.473825Z  INFO db: Migrating DatabaseVersionKeyV0 of ln to DatabaseVersionKey. Version: 1
2024-03-16T21:45:20.473877Z  INFO db: Migrating DatabaseVersionKeyV0 of mint to DatabaseVersionKey. Version: 0
2024-03-16T21:45:20.473899Z  INFO db: Migrating DatabaseVersionKeyV0 of wallet to DatabaseVersionKey. Version:
 0
2024-03-16T21:45:20.473917Z  INFO db: Migrating DatabaseVersionKeyV0 of meta to DatabaseVersionKey. Version: 0
2

and it seems it actually never completes it, because there is nothing else to migrate and transaction is terminated early, without completing this one migration. In other words - it does some modifications, than never calls the commit.

@dpc dpc requested a review from a team as a code owner March 16, 2024 21:53
dpc added 4 commits March 16, 2024 16:59
I've noticed the `fedimint-cli` keeps printing over and over:

```
2024-03-16T21:45:20.473825Z  INFO db: Migrating DatabaseVersionKeyV0 of ln to DatabaseVersionKey. Version: 1
2024-03-16T21:45:20.473877Z  INFO db: Migrating DatabaseVersionKeyV0 of mint to DatabaseVersionKey. Version: 0
2024-03-16T21:45:20.473899Z  INFO db: Migrating DatabaseVersionKeyV0 of wallet to DatabaseVersionKey. Version:
 0
2024-03-16T21:45:20.473917Z  INFO db: Migrating DatabaseVersionKeyV0 of meta to DatabaseVersionKey. Version: 0
2
```

and it seems it actually never completes it, because there is nothing
else to migrate and transaction is terminated early, without completing
this one migration.
The way the existing code worked, is that it first opened the database
in the temporary directory and then immediately deleted this directory
(by droping the `TempDir`) that seems to work OK only as long as the
temporary `Database` doesn't do any writes to the dir (commits).
@dpc dpc force-pushed the 24-03-16-db-version-migratin-fix branch from c0dfa18 to 57fab1e Compare March 16, 2024 23:59
@elsirion elsirion self-requested a review March 18, 2024 16:08
@dpc dpc enabled auto-merge March 18, 2024 19:25
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well-structured PR, was a joy to read!

@dpc dpc added this pull request to the merge queue Mar 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 19, 2024
@elsirion elsirion added this pull request to the merge queue Mar 19, 2024
Merged via the queue into fedimint:master with commit 0eff575 Mar 19, 2024
20 checks passed
@dpc dpc deleted the 24-03-16-db-version-migratin-fix branch March 19, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants