[sql-67] multi: tombstone bbolt databases on successful SQL migration#1272
Conversation
ViktorT-11
added
the
no-changelog
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a safety mechanism to ensure that legacy bbolt database files are properly deprecated after a successful migration to a SQL backend. By adding a tombstone marker to these files, the system prevents users from inadvertently continuing to use the old bbolt backend, which could lead to data inconsistency. The changes also ensure that if a user removes their SQL database, they can still re-run the migration process using the existing tombstoned files, maintaining flexibility while enforcing data integrity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a deprecation mechanism for legacy bbolt databases after SQL migration, ensuring that stale files are tombstoned and blocked from normal startup. It also renames migration structures from 'streams' to 'sets' and updates integration tests to verify the new deprecation flow. Review feedback identifies opportunities to optimize startup by avoiding redundant database opens and suggests adding file existence checks to prevent creating unnecessary empty files during deprecation.
ViktorT-11
force-pushed
the
2026-03-deprecate-kvdb-on-mig
branch
2 times, most recently
from
6a9cb51 to
ca6685c
Compare
|
@ellemouton: review reminder |
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Looks great conceptually 🙏, only had some minor comments.
ViktorT-11
force-pushed
the
2026-03-deprecate-kvdb-on-mig
branch
from
ca6685c to
bfb0dd8
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
LGTM 🎉 (also saw that the backward compatibility test ran).
| // committed so a failed SQL migration cannot strand the user with an unusable | ||
| // kvdb backend. | ||
| func deprecateKVDBStores(dbDir string) error { | ||
| err := accounts.DeprecateKVDB( |
There was a problem hiding this comment.
We could use errors.Join in the end to try all deprecations?
There was a problem hiding this comment.
agreed. atm it could land in a state where first one succeeds, second fails and then third isnt attempted.
There was a problem hiding this comment.
Thanks for the suggestion, addressed!
|
Thanks for the review @bitromortac! I'll address your latest feedback after the next review round (from @ellemouton) 🎉 |
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
exciting!
left some comments
|
|
||
| err := hn.cmd.Wait() | ||
| if err != nil { | ||
| litdError <- fmt.Errorf("%v\n%v\n", err, errb.String()) | ||
| } | ||
|
|
||
| // Signal any onlookers that this process has exited. | ||
| close(hn.processExit) | ||
|
|
||
| // Make sure log file is closed and renamed if necessary. | ||
| finalizeLogfile() | ||
|
|
There was a problem hiding this comment.
the code should document why the error check is so much later on else it looks like a bug & also at a glance just looks like an unchecked error
There was a problem hiding this comment.
Updated with a doc, also buffered the channel.
| // committed so a failed SQL migration cannot strand the user with an unusable | ||
| // kvdb backend. | ||
| func deprecateKVDBStores(dbDir string) error { | ||
| err := accounts.DeprecateKVDB( |
There was a problem hiding this comment.
agreed. atm it could land in a state where first one succeeds, second fails and then third isnt attempted.
| @@ -0,0 +1,100 @@ | |||
| package accounts | |||
There was a problem hiding this comment.
i dont think it is clear why the tomb-stoning is done differently for accounts vs session/firewall packages. i think this could be made clearer (if there is a reason).
Ideally there is one Deprecation helper/set of helpers and all three packages just call that (can just pass in a top-level bucket?). or is the idea that each file's keys are kept in that package? if so , can let the helper take in a "tombstone key prefix" perhaps.
See how LND does tombstoneing (invoices package for example).
There was a problem hiding this comment.
Good idea! Updated to add a new tombstone package, which is used across the packages instead.
Also updated to use the same approach lnd uses for tombstoneing
| // still be opened unexpectedly. | ||
| err = deprecateKVDBStores(filepath.Dir(macPath)) | ||
| if err != nil { | ||
| log.Errorf("CRITICAL: kvdb -> SQL migration "+ |
There was a problem hiding this comment.
is there a way for us to retry later on? how does lnd do it?
There was a problem hiding this comment.
I did not update this.
IMO, the way you'd need to implement it is that you'd need to add a flag in the sql db that signals if the kvdb has been tombstoned or not, which is initialized as false. We'd then retry the tombstoneing until the flag is set to true, which we'd do after a successful tombstoneing, or if the kdvb files do not exist.
That would requiring adding a separate table for this in the sql db, which I think might been seen as overengineering for an issue that potentially never occur for any users, and if it does, you could argue that the outcome not that bad.
If you think it makes sense to add that tracking table in sql, I'm open to doing so though, unless you have a better idea of how to solve it!
The current implementation in lnd is even less complete than the above:
// Set the invoice bucket tombstone to indicate
// that the migration has been completed.
//
// TODO(ziggie): The tombstone is currently
// set inside the SQL transaction callback,
// which is fragile: if the SQL transaction
// is retried (e.g. on a serialization
// error), the KV tombstone is written before
// the SQL commit is confirmed. Move this to
// run after ApplyAllMigrations returns so
// the tombstone is only set once the
// migration is durably committed.
d.logger.Debugf("Setting invoice bucket " +
"tombstone")
//nolint:ll
return dbs.ChanStateDB.SetInvoiceBucketTombstone()
bfb0dd8 to
7a6710a
Compare
|
Thank you so much for the detailed review @ellemouton! Addressed your feedback 🔥 I'll fix the linter issue & #1272 (comment) on the next review round. |
Mark the legacy kvdb stores as deprecated once the kvdb -> SQL migration commits successfully. This prevents normal bbolt startup from reopening accounts.db, session.db, or rules.db after their data has already been migrated. Add explicit deprecation checks to the three kvdb store open paths and provide migration-only constructors that can still reopen deprecated files when the SQL database is deleted or downgraded and the migration must be rerun. Use store-specific tombstones for the deprecation markers and add tests that verify deprecated stores are rejected while migration reruns continue to work.
Close the process exit signal before forwarding litd startup errors from the wait goroutine. This lets the harness observe that the process has already exited even when no receiver is currently ready on the error channel. Use a non-blocking send for the captured process error so failed startup paths do not hang the goroutine while holding back log finalization or process exit handling. This will be needed for the upcoming commit which adds itest coverage of deprecated kvdb databases.
Expand the kvdb -> SQL migration itest to cover the full post-migration startup behavior, not just the initial data copy and SQL assertions. Verify that bbolt startup is blocked once kvdb files are deprecated, that deleting the SQL database reruns the migration successfully, and that older litd binaries still fail to start against deprecated kvdb files. Also add ordered blocker checks for the deprecated kvdb files so the test shows startup fails first on accounts.db, then session.db, and finally rules.db as earlier files are removed.
ViktorT-11
force-pushed
the
2026-03-deprecate-kvdb-on-mig
branch
from
7a6710a to
d519953
Compare
|
Thanks for the reviews 🎉! |
5 participants
Based on #1266
This PR tombstones/deprecates the different bbolt db files once a
litdnode has successfully migrated from kvdb -> SQL.The files are intentionally kept with the same name, but are updated by adding an extra bucket in the different files, so that startup of
litdwill fail if the user decides to change back theirdatabasebackendtobboltafter the migration.The motivation for this is to ensure that the user only has one source of data, and so that the user can't continue to operate with a
bboltdatabase and add new data to it after the migration, as that data cannot be migrated over to the users new SQL database if we did allow the user to continue to run with thebboltbackend.The tombstone is intentionally designed so that the
accounts.dbwill cause a reading failure if the user decides to downgrade theirlitdnode to a version prior to the tombstone logic existing. If the user runs alitdversion which includes the tombstone logic, the startup will fail when reading thesession.db&rules.dbfiles as well, in the scenario that for example the user deletes theiraccounts.dbfile, but keeps their tombstonedsession.db&rules.dbfiles.Additionally, the logic is designed so that in case the user deletes their SQL database file after migration, they can still re-run a new kvdb -> SQL migration with their old tombstoned
bboltdatabase files.