Skip to content

[sql-48] session: sort MacaroonRecipe.caveats in migration #1129

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ViktorTigerstrom
Copy link
Contributor

Addresses the CI flake first discovered in #1126, run:
https://github.com/lightninglabs/lightning-terminal/actions/runs/16777826198/job/47508140669

In the kvdb to sql migration, if there have been caveats set for the MacaroonRecipe, the order of the postgres db caveats will in very rare cases differ from the kv store caveats. Therefore, we sort both the kv and sql caveats by their ID, so that we can compare them in a deterministic way.

I tried creating a deterministic test that triggers this issue, but wasn't successful. The best semi deterministic way to trigger this that I've found is to increased the number of sessions in the randomizedSessions test to high number like 5000, and then run this on a postgres backend. That triggers the issue most of the times.

@ellemouton & @bitromortac, even though we haven't seen this behaviour in any other migration, this made me think that it won't hurt too much to add sorting to all migrations where the object we're comparing includes an array of some sort. If you agree with my reasoning, I'll push another PR that does add sorting to all such cases.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @ViktorTigerstrom, 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 addresses a known CI flakiness issue related to the kvdb to sql migration process. The core problem stemmed from non-deterministic ordering of MacaroonRecipe caveats when comparing data between the KV store and the SQL database. By implementing a sorting mechanism for these caveats, the PR ensures consistent and deterministic comparisons, thereby stabilizing the migration and preventing future CI failures.

Highlights

  • Migration Determinism: Introduced sorting for MacaroonRecipe.Caveats within the overrideMacaroonRecipe function. This ensures that both the KV store and SQL database versions of caveats are sorted by their Id before comparison, resolving potential non-deterministic ordering issues that could lead to CI flakes during the kvdb to sql migration.
  • Dependency Addition: The standard sort package was imported into session/sql_migration.go to facilitate the new sorting logic for MacaroonRecipe.Caveats.
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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 or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@ViktorTigerstrom ViktorTigerstrom added the no-changelog This PR is does not require a release notes entry label Aug 7, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The code changes introduce sorting of MacaroonRecipe.caveats to ensure deterministic comparison during session migration. The review suggests adding a nil check for migratedSession.MacaroonRecipe before accessing its fields and sorting MacaroonRecipe.Permissions for consistent comparison.

In the kvdb to sql migration, if there have been caveats set for the
MacaroonRecipe, the order of the postgres db caveats will in very rare
cases differ from the kv store caveats. Therefore, we sort both the kv
and sql caveats by their ID, so that we can compare them in a
deterministic way.
@ViktorTigerstrom ViktorTigerstrom force-pushed the 2025-08-session-migration-ceavat-order-fix branch from 3c6974b to d3eb3cd Compare August 7, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This PR is does not require a release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant