fix: close dangling session, migration corrections

[CyanVoxel]

Summary

This PR fixes issues caused by a missing session context manager in the all_entries() method in conjunction with the new autocommit=False DB parameter, and makes some tweaks to the migration refactors done in #1432 and #1456, most notably removing static methods in the Library class and removing local imports ^{because I'm an idiot and forgot that I didn't completely review #1456 before signing off and pulling it}

Closes #1447, Closes #1467

Tasks Completed

• Platforms Tested:
  • Windows x86
  • Windows ARM
  • macOS x86
  • macOS ARM
  • Linux x86
  • Linux ARM
• Tested For:
  • Basic functionality
  • PyInstaller executable

[Computerdores]

Why make that non-static?

[Computerdores]

I had originally passed the library path and engine because the engine argument will be removed when the migrations are made sql alchemy independent (and thus fully independent of the model that the to-be-migrated lib doesn't yet follow). Once that is done the parameter would only be the library path which is a much looser coupling then passing the full library in¹.

For that reason I feel it is undesirable to pass the entire library to the constructor here.
Also, afaict it isn't necessary.

Note: Afaict the statements setting library_dir in Library.create_sqlite_library Library.open_sqlite_library can also be removed once this is reverted.

Footnotes

1. We want the coupling to be as loose as possible here, because the logic in the Library class will be for the current schema while the migrations deal with old schemas and so we can't (and shouldn't where we still can) rely on that logic. ↩

[Computerdores]

I like annotating the methods with the type for fmt_log, but I think it would be better to define something like LoggingMethod = Callable[[str], str] and to then set fmt_log: LoggingMethod

[Computerdores]

I had made this static since the idea is to call it when the library isn't loaded yet (and can't be loaded at all due to needing to be migrated), that's why it was static and why I still think it should be here.

We could maybe also just move this to the DBMigrations to be done as the first step of DBMigrations.run.

[CyanVoxel]

The save_library_backup_to_disk() method doesn't require a library to be loaded, it just uses the Library instance's self.library_dir, which I tweaked to be set before the migrations are run in open_sqlite_library().

We could maybe also just move this to the DBMigrations to be done as the first step of DBMigrations.run.

This method is also used outside of the migrations, like in the UI - so there still needs to be a non-static version of it present in the Library class for instances to use without passing the argument of the instance's own library_dir.

[Computerdores]

The save_library_backup_to_disk() method doesn't require a library to be loaded, it just uses the Library instance's self.library_dir, which I tweaked to be set before the migrations are run in open_sqlite_library().

Yeah this is exactly what I meant; normally the library_dir means "the directory of the currently open library" but here it effectively is "the directory of the library that should be backed up" (unless a library is actually open, in which the original meaning is correct again).

We could maybe also just move this to the DBMigrations to be done as the first step of DBMigrations.run.

This method is also used outside of the migrations, like in the UI - so there still needs to be a non-static version of it present in the Library class for instances to use without passing the argument of the instance's own library_dir.

I think having a static method and a non-static method that calls the static one (with self.library_dir as the param) would be best then.

[Computerdores]

This split existed, because it is being used in the migrations where the existing engine should be used to ensure consistency irrespective of future changes to the migrations.

However, I think this can just be moved to the migrations entirely as the only other use can be removed.
(It is in LibraryInfoWindow where get_version(DB_VERSION_CURRENT_KEY) is contrasted to DB_VERSION, which is useless because the migrations fail unless get_version(DB_VERSION_CURRENT_KEY) == DB_VERSION meaning that can be removed.)

[CyanVoxel]

This split existed, because it is being used in the migrations where the existing engine should be used to ensure consistency irrespective of future changes to the migrations.

Where might an inconsistency arise that would be solved by passing a different engine to build a session from?

However, I think this can just be moved to the migrations entirely as the only other use can be removed.
(It is in LibraryInfoWindow where get_version(DB_VERSION_CURRENT_KEY) is contrasted to DB_VERSION, which is useless because the migrations fail unless get_version(DB_VERSION_CURRENT_KEY) == DB_VERSION meaning that can be removed.)

The current UI-facing use of this is working as intended, where it shows the current loaded DB version in comparison to the program's currently known DB_VERSION, which is useful for telling if you have a DB loaded with a minor version greater than the TagStudio version that's opening it. I'd also like to not gate the _get_version() method to the migrations, as that would limit future uses in the UI.

Perhaps the "static" version of this that includes the engine parameter and inspection conditional could be moved to the migrations class, while a simpler instance method that just uses the currently expected logic could remain in the library file? Though that's a tiny bit of code duplication

[Computerdores]

This split existed, because it is being used in the migrations where the existing engine should be used to ensure consistency irrespective of future changes to the migrations.

Where might an inconsistency arise that would be solved by passing a different engine to build a session from?

The engine is specific to the DB that is currently open, so if we were to e.g. allow a user to bulk migrate libraries while a different library is open, then this get_version method would cause the migrations to see the incorrect version and not migrate anything because it would see the version of the currently open library and not the one of the DB being migrated.

However, I think this can just be moved to the migrations entirely as the only other use can be removed.
(It is in LibraryInfoWindow where get_version(DB_VERSION_CURRENT_KEY) is contrasted to DB_VERSION, which is useless because the migrations fail unless get_version(DB_VERSION_CURRENT_KEY) == DB_VERSION meaning that can be removed.)

The current UI-facing use of this is working as intended, where it shows the current loaded DB version in comparison to the program's currently known DB_VERSION, which is useful for telling if you have a DB loaded with a minor version greater than the TagStudio version that's opening it.

Good point, I missed that.

I'd also like to not gate the _get_version() method to the migrations, as that would limit future uses in the UI.

Perhaps the "static" version of this that includes the engine parameter and inspection conditional could be moved to the migrations class, while a simpler instance method that just uses the currently expected logic could remain in the library file? Though that's a tiny bit of code duplication

Yes, I think that would be best. That way get_version is basically a one-liner and the complicated DB introspection logic can be relegated to the migrations.
Also, uncoupling the migrations from sqlalchemy would probably require that anyway, so we might as well do it now.

[Computerdores]

This PR fixes issues caused by a missing session context manager in the all_entries() method in conjunction with the new autocommit=False DB parameter

RE the actual fix: I am not quite sure why this fixes anything, because the original setup was not missing any context managers at all. The _all_entries method was simply shifting the burden of opening the context manager to the caller instead of opening its own (which would have caused errors in the migrations due to two DB sessions being opened at the same time). My best guess is that because the code now used in the migrations instead is much simpler, which makes me suspicious that the same error might still occur where all_entries is still used.

[CyanVoxel]

Why make[...] non-static?

For the non-static reversions, this was mostly motivated by the idea that the Library is not intended to be a singleton class - each instance of the Library class should be completely self-contained and methods for the class that affect instanced versions should only affect data for said instance. While making some methods static didn't necessarily turn the Library class into a singleton, it still separated the self-contained nature of the Library instance by requiring a session to be independently supplied (which raised a deeper issue with the all_entries() method). This also got rid of the local imports in migrations.py, which I had missed were added.

RE the actual fix: I am not quite sure why this fixes anything, because the original setup was not missing any context managers at all. The _all_entries method was simply shifting the burden of opening the context manager to the caller instead of opening its own (which would have caused errors in the migrations due to two DB sessions being opened at the same time). My best guess is that because the code now used in the migrations instead is much simpler, which makes me suspicious that the same error might still occur where all_entries is still used.

When turning the all_entries() method back into a non-static class an using the session associated with the Library class instance, that fixed the DB lock that was occurring presumably due to a mismatched instance being passed from the migrations that was holding the lock. Following the thread of this fix raised issues with migration 9, where the results from all_entries() would not contain the flushed column that gets added in 9, which is where I switched to querying the DB directly via the migration's session instead of using the full all_entries() function that didn't contain the flushed changes (I'm pretty sure this is a reason I had the migrations split in a weird order initially, as a poor workaround). These changes also enforce using Library instances as a single unit instead of passing around session contexts that can become disjointed due to SQLAlchemy being just wonderful to work with.

So a lot of the static reversions weren't 100% necessary to pull off this fix, but sort of went hand-in-hand with it and also reflect my own intentions for how the Library class should be used.

[Computerdores]

Why make[...] non-static?

For the non-static reversions, this was mostly motivated by the idea that the Library is not intended to be a singleton class - each instance of the Library class should be completely self-contained and methods for the class that affect instanced versions should only affect data for said instance.

This only applies to the all_entries method though (which I agree shouldn't have been static, no idea why I did that ^^).
Neither _get_engine nor save_library_backup_to_disk affect (or access) the data of the Library instance...

Also, the semantics of save_library_backup_to_disk are to make a backup of the library DB found at a certain path, not to make a backup of the currently opened library. And making the method non-static implies that it could access anything available on the instance when a library is accessed, which could lead to problems, because in order to call it from the migrations only the library path is set and the library is actually opened (which would of course not be possible). This is why I have to object to making save_library_backup_to_disk an instance method.

When turning the all_entries() method back into a non-static class an using the session associated with the Library class instance,

The session is in no way associated with the Library instance

that fixed the DB lock that was occurring presumably due to a mismatched instance being passed from the migrations that was holding the lock.

If you compare how the session is built now and how it was built then, they are exactly identical, so this cannot be the reason afaict.

These changes also enforce using Library instances as a single unit instead of passing around session contexts that can become disjointed

Not quite sure what you mean here

[CyanVoxel]

Also, the semantics of save_library_backup_to_disk are to make a backup of the library DB found at a certain path, not to make a backup of the currently opened library. And making the method non-static implies that it could access anything available on the instance when a library is accessed, which could lead to problems, because in order to call it from the migrations only the library path is set and the library is actually opened (which would of course not be possible). This is why I have to object to making save_library_backup_to_disk an instance method.

The save_library_backup_to_disk() was previously an instance method and used as such outside of migrations (e.g. the backup_library() method in ts_qt.py). There can be special cases for things that the new migration system needs, but it shouldn't be assumed that these functions aren't being used elsewhere where the expectation is to just have the Library instance, and then tell that library to save a backup using its own library_dir.

The session is in no way associated with the Library instance

I'm referring to how the session context managers in the Library class are constructed using with Session(self.engine) as session:, which use the self.engine variable inside the particular Library instance. Reverting the all_entries() method to be non-static and restoring the original context manager with self.engine fixes the lock on the DB created by "something else, somewhere else".

If you compare how the session is built now and how it was built then, they are exactly identical, so this cannot be the reason afaict.

If you've got an alternate explanation for #1447 I'm all ears, but from all the investigating and testing I've done it really seems like the session currently is getting detached or desycned or something from the rest of the Library, causing a lock on the DB because there's now something outside the scope of the expected session that's using it (in conjunction with using autocommit=False). And restoring the original context manager using the instantiated Library class's self.engine resolves that lock issue without having to revert the recommended autocommit parameter.

Changes made from there were either due to other obscured issues like the DB9 migration flushing and/or due to wanting to enforce a particular pattern for accessing library methods.

[CyanVoxel]

@Computerdores Unfortunately my hands are kind of tied with this, where the reversions of (at least some of) the static methods plus the reversion to use the session associated with a Library instance is fixing this critical issue that needs to ship as soon as it can, and I don't have evidence of an unrelated cause for all this.

The closest I've been able to glean is a similar issue I fixed in dd00f4d where colors couldn't be added due to a dormant double session issue that started causing a DB lock, but I couldn't find evidence of a double session going on here. SQLAlchemy doesn't exactly make it straightforward to trace locks, unfortunately...

We can revisit some of the static discussions and migration parameters in the future, but right now there needs to be a fix for #1432/#1456 and this is the only one, which was created on a best-effort basis for this nebulous ORM breakage.

[CyanVoxel]

I believe @racerand on Discord has uncovered the fundamental cause of the issue: when all_entries() was split into two methods, the instanced one that wraps a session had return Library._all_entries(session, with_joins) when it should have had yield from Library._all_entries(session, with_joins) to properly differ to the Iterator yielded from the static _all_entries().

This helps quell my concerns about passing sessions around, and with seeing the double session issue in dd00f4d plus a clear example of an internal need to shield library logic from double sessions in #1470, I've gained a greater context for what kinds of shifts are needed in the Library class design - keeping in mind that the session/cursor concept will likely stay after moving from SQLAlchemy to raw sqlite3.

I think there's a growing split in what the Library class is thought to be used for now, when previously it was purely a "Class for the Library object, and all CRUD operations made upon it", and having static methods that are intended to be used publicly or psuedo-publicly that don't relate to a Library instance felt inherently against this - but more of these types of operations are becoming necessary, including by Library instances themselves. One direction things could take is splitting the static, non-context-managed logic out into a separate class (LibraryOps or something) that is then referenced by both the Migrations class and Library instances whenever they need to.

---

So with that in mind, I think I'm going to do a quick commit to main with the yield from change, make a small PR with some of the small misc fixes and cleanup included in here, and leave the rest of the big ticket changes for a lower priority PR targeting 9.6.4.

[Computerdores]

I believe @racerand on Discord has uncovered the fundamental cause of the issue: when all_entries() was split into two methods, the instanced one that wraps a session had return Library._all_entries(session, with_joins) when it should have had yield from Library._all_entries(session, with_joins) to properly differ to the Iterator yielded from the static _all_entries().

So I wasn't going insane! Although I would have expected a warning from pyright for this kind of thing, but whatever

I think there's a growing split in what the Library class is thought to be used for now, when previously it was purely a "Class for the Library object, and all CRUD operations made upon it", and having static methods that are intended to be used publicly or psuedo-publicly that don't relate to a Library instance felt inherently against this - but more of these types of operations are becoming necessary, including by Library instances themselves. One direction things could take is splitting the static, non-context-managed logic out into a separate class (LibraryOps or something) that is then referenced by both the Migrations class and Library instances whenever they need to.

I would suggest calling it _LibraryOps and putting it somewhere in tagstudio.core.library.alchemy to communicate that it isn't meant for use outside of that.

So with that in mind, I think I'm going to do a quick commit to main with the yield from change, make a small PR with some of the small misc fixes and cleanup included in here, and leave the rest of the big ticket changes for a lower priority PR targeting 9.6.4.

Good decision, even before racerand's insight; much as I dislike pushing a fix I don't understand (must be the security part of my brain) I also think that it is more important to have it working than for the code to be beautiful.