Skip to content

fix: close dangling session, migration corrections #1471

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.

Sign up for GitHub

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
CyanVoxel wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: close dangling session, migration corrections #1471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 36 additions & 48 deletions src/tagstudio/core/library/alchemy/library.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,6 @@ def close(self):
if self.engine:
self.engine.dispose()
self.library_dir = None
self.folder = None
self.included_files = set()

self.dupe_entries_count = -1
Expand Down Expand Up @@ -378,8 +377,7 @@ def open_library(self, library_dir: Path, in_memory: bool = False) -> LibrarySta

return self.open_sqlite_library(library_dir, in_memory)

@staticmethod
def __get_engine(library_dir: Path, in_memory: bool, sql_filename: str):
def _get_engine(self, library_dir: Path, in_memory: bool, sql_filename: str):

@Computerdores Computerdores Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why make that non-static?

connection_string = URL.create(
drivername="sqlite",
database=(
Expand Down Expand Up @@ -407,7 +405,7 @@ def __get_engine(library_dir: Path, in_memory: bool, sql_filename: str):
def create_sqlite_library(
self, library_dir: Path, in_memory: bool, sql_filename: str = SQL_FILENAME
) -> LibraryStatus:
self.engine = self.__get_engine(library_dir, in_memory, sql_filename)
self.engine = self._get_engine(library_dir, in_memory, sql_filename)

logger.info(
"[Library] Opening SQLite Library",
Expand Down Expand Up @@ -505,21 +503,21 @@ def open_sqlite_library(
) -> LibraryStatus:
logger.info("[Library] Opening SQLite Library", library_dir=library_dir)

self.engine = self.__get_engine(library_dir, in_memory, sql_filename)
self.engine = self._get_engine(library_dir, in_memory, sql_filename)
self.library_dir = library_dir

try:
migrations = DBMigrations(library_dir, self.engine)
migrations = DBMigrations(self)

# save backup if patches will be applied
if migrations.required:
Library.save_library_backup_to_disk(library_dir)
self.save_library_backup_to_disk()

migrations.run()
except MigrationError as e:
self.library_dir = None
return LibraryStatus(success=False, message=e.args[0])

# everything is fine, set the library path
self.library_dir = library_dir
return LibraryStatus(success=True, library_path=library_dir)

@property
Expand Down Expand Up @@ -669,37 +667,32 @@ def entries_count(self) -> int:
with Session(self.engine) as session:
return unwrap(session.scalar(select(func.count(Entry.id))))

@staticmethod
def _all_entries(session: Session, with_joins: bool = False) -> Iterator[Entry]:
def all_entries(self, with_joins: bool = False) -> Iterator[Entry]:
"""Load entries without joins."""
stmt = select(Entry)
if with_joins:
# load Entry with all joins and all tags
stmt = (
stmt.outerjoin(Entry.text_fields)
.outerjoin(Entry.datetime_fields)
.outerjoin(Entry.tags)
)
stmt = stmt.options(
contains_eager(Entry.text_fields),
contains_eager(Entry.datetime_fields),
contains_eager(Entry.tags),
)

stmt = stmt.distinct()
with Session(self.engine) as session:
stmt = select(Entry)
if with_joins:
# load Entry with all joins and all tags
stmt = (
stmt.outerjoin(Entry.text_fields)
.outerjoin(Entry.datetime_fields)
.outerjoin(Entry.tags)
)
stmt = stmt.options(
contains_eager(Entry.text_fields),
contains_eager(Entry.datetime_fields),
contains_eager(Entry.tags),
)

entries = session.execute(stmt).scalars()
if with_joins:
entries = entries.unique()
stmt = stmt.distinct()

for entry in entries:
yield entry
session.expunge(entry)
entries = session.execute(stmt).scalars()
if with_joins:
entries = entries.unique()

def all_entries(self, with_joins: bool = False) -> Iterator[Entry]:
"""Load entries without joins."""
with Session(self.engine) as session:
return Library._all_entries(session, with_joins)
for entry in entries:
yield entry
session.expunge(entry)

@property
def tags(self) -> list[Tag]:
Expand Down Expand Up @@ -1447,17 +1440,16 @@ def delete_color(self, color: TagColorGroup):
session.rollback()
return None

@staticmethod
def save_library_backup_to_disk(library_dir: Path) -> Path:
Comment on lines -1450 to -1451

@Computerdores Computerdores Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 CyanVoxel Aug 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 Computerdores Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

assert isinstance(library_dir, Path)
makedirs(str(library_dir / TS_FOLDER_NAME / BACKUP_FOLDER_NAME), exist_ok=True)
def save_library_backup_to_disk(self) -> Path:
assert isinstance(self.library_dir, Path)
makedirs(str(self.library_dir / TS_FOLDER_NAME / BACKUP_FOLDER_NAME), exist_ok=True)

filename = f"ts_library_backup_{datetime.now(UTC).strftime('%Y_%m_%d_%H%M%S')}.sqlite"

target_path = library_dir / TS_FOLDER_NAME / BACKUP_FOLDER_NAME / filename
target_path = self.library_dir / TS_FOLDER_NAME / BACKUP_FOLDER_NAME / filename

shutil.copy2(
library_dir / TS_FOLDER_NAME / SQL_FILENAME,
self.library_dir / TS_FOLDER_NAME / SQL_FILENAME,
target_path,
)

Expand Down Expand Up @@ -1749,12 +1741,8 @@ def get_version(self, key: str) -> int:
Args:
key(str): The key for the name of the version type to set.
"""
return Library._get_version(self.engine, key)

@staticmethod
def _get_version(engine, key: str) -> int:
with Session(engine) as session:
engine = sqlalchemy.inspect(engine)
with Session(self.engine) as session:
engine = sqlalchemy.inspect(self.engine)
try:
# "Version" table added in DB_VERSION 101
if engine and engine.has_table("versions"):
Expand Down
59 changes: 30 additions & 29 deletions src/tagstudio/core/library/alchemy/migrations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@

from collections.abc import Callable
from pathlib import Path
from typing import override
from typing import TYPE_CHECKING, override

import structlog
import ujson
from sqlalchemy import Engine, and_, delete, select, text, update
from sqlalchemy import and_, delete, select, text, update
from sqlalchemy.orm import Session

from tagstudio.core.constants import IGNORE_NAME, TAG_ARCHIVED, TS_FOLDER_NAME
Expand All @@ -21,11 +21,14 @@
)
from tagstudio.core.library.alchemy.fields import LEGACY_FIELD_MAP, DatetimeField, TextField
from tagstudio.core.library.alchemy.joins import TagParent
from tagstudio.core.library.alchemy.models import Tag, TagColorGroup, Version
from tagstudio.core.library.alchemy.models import Entry, Tag, TagColorGroup, Version
from tagstudio.core.library.ignore import migrate_ext_list
from tagstudio.core.utils.types import unwrap
from tagstudio.qt.translations import Translations

if TYPE_CHECKING:
from tagstudio.core.library.alchemy.library import Library

logger = structlog.get_logger(__name__)


Expand All @@ -38,20 +41,19 @@ class DBMigration:
initial_version: int | None = None

@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log: Callable[[str], str]):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]) -> None: # pyright: ignore[reportUnusedParameter]
raise NotImplementedError


class DBMigrations:
def __init__(self, library_dir: Path, engine: Engine) -> None:
from tagstudio.core.library.alchemy.library import Library
def __init__(self, library: "Library") -> None:

@Computerdores Computerdores Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 in1.

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. ↩


self.library_dir = library_dir
self.engine = engine
self.lib = library
self.engine = self.lib.engine

# Don't check DB version when creating new library
self.loaded_db_version = Library._get_version(engine, DB_VERSION_CURRENT_KEY)
self.initial_db_version = Library._get_version(engine, DB_VERSION_INITIAL_KEY)
self.loaded_db_version = self.lib.get_version(DB_VERSION_CURRENT_KEY)
self.initial_db_version = self.lib.get_version(DB_VERSION_INITIAL_KEY)

# ======================== Library Database Version Checking =======================
# DB_VERSION 6 is the first supported SQLite DB version.
Expand Down Expand Up @@ -107,7 +109,7 @@ def run(self):
# any error causes transaction to rollback
migration.run(
session,
self.library_dir,
self.lib,
lambda msg, v=migration.version: f"[Library][Migration][{v}] {msg}",
)
self.loaded_db_version = migration.version
Expand Down Expand Up @@ -146,7 +148,7 @@ class MigrationTo7(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):

@Computerdores Computerdores Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

"""Migrate DB from DB_VERSION 6 to 7."""
logger.info(fmt_log("Applying patches to DB_VERSION: 6 library..."))
# Repair tags that may have a disambiguation_id pointing towards a deleted tag.
Expand All @@ -166,7 +168,7 @@ class MigrationTo8(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB from DB_VERSION 7 to 8."""
# Add the missing color_border column to the TagColorGroups table.
session.execute(
Expand Down Expand Up @@ -219,7 +221,7 @@ class MigrationTo9(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB from DB_VERSION 8 to 9."""
# Apply database schema changes
add_filename_column = text(
Expand All @@ -230,9 +232,8 @@ def run(cls, session: Session, library_dir: Path, fmt_log):
logger.info(fmt_log("Added filename column to entries table"))

# Populate the new filename column.
from tagstudio.core.library.alchemy.library import Library

for entry in Library._all_entries(session):
entries = session.execute(select(Entry).distinct()).scalars()
for entry in entries:
Comment thread
CyanVoxel marked this conversation as resolved.
entry.filename = entry.path.name
session.merge(entry)
session.flush()
Expand All @@ -244,7 +245,7 @@ class MigrationTo100(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB to DB_VERSION 100."""
# Repair parent-child tag relationships that are the wrong way around.
stmt = update(TagParent).values(
Expand All @@ -261,7 +262,7 @@ class MigrationTo101(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB to DB_VERSION 101."""
# Create versions table
session.execute(
Expand All @@ -284,7 +285,7 @@ class MigrationTo102(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB to DB_VERSION 102."""
# delete TagParents with a dangling parent reference
stmt = delete(TagParent).where(TagParent.parent_id.not_in(select(Tag.id).distinct()))
Expand All @@ -298,7 +299,7 @@ class MigrationTo103(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB from DB_VERSION 102 to 103."""
# add the new hidden column for tags
session.execute(text("ALTER TABLE tags ADD COLUMN is_hidden BOOLEAN NOT NULL DEFAULT 0"))
Expand All @@ -316,17 +317,17 @@ class MigrationTo104(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB from DB_VERSION 103 to 104."""
# Convert file extension list to ts_ignore file, if a .ts_ignore file does not exist
cls.__migrate_sql_to_ts_ignore(session, library_dir)
cls.__migrate_sql_to_ts_ignore(session, library)
session.execute(text("DROP TABLE preferences"))
session.flush()

@classmethod
def __migrate_sql_to_ts_ignore(cls, session: Session, library_dir: Path):
def __migrate_sql_to_ts_ignore(cls, session: Session, library: "Library"):
# Do not continue if existing '.ts_ignore' file is found
ts_ignore = library_dir / TS_FOLDER_NAME / IGNORE_NAME
ts_ignore = unwrap(library.library_dir) / TS_FOLDER_NAME / IGNORE_NAME
if Path(ts_ignore).exists():
return

Expand All @@ -349,7 +350,7 @@ class MigrationTo200(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB to DB_VERSION 200."""
# Drop unused 'boolean_fields' and 'value_type' tables
logger.info(fmt_log("Dropping boolean_fields and value_type tables..."))
Expand Down Expand Up @@ -463,7 +464,7 @@ class MigrationTo201(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB to DB_VERSION 201."""
create_text_fields_table = text("""
CREATE TABLE text_fields_new (
Expand Down Expand Up @@ -519,7 +520,7 @@ class MigrationTo202(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
"""Migrate DB to DB_VERSION 202."""
stmt = delete(TagParent).where(TagParent.child_id.not_in(select(Tag.id).distinct()))
session.execute(stmt)
Expand All @@ -532,7 +533,7 @@ class MigrationTo300(DBMigration):

@override
@classmethod
def run(cls, session: Session, library_dir: Path, fmt_log):
def run(cls, session: Session, library: "Library", fmt_log: Callable[[str], str]):
## remove folder_id column from entries table
# create new table in the desired scheme (without folder_id column)
session.execute(
Expand Down
8 changes: 5 additions & 3 deletions src/tagstudio/qt/ts_qt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,7 @@ def backup_library(self):
logger.info("Backing Up Library...")
self.main_window.status_bar.showMessage(Translations["status.library_backup_in_progress"])
start_time = time.time()
target_path = Library.save_library_backup_to_disk(unwrap(self.lib.library_dir))
target_path = self.lib.save_library_backup_to_disk()
end_time = time.time()
self.main_window.status_bar.showMessage(
Translations.format(
Expand Down Expand Up @@ -1651,7 +1651,8 @@ def open_library(self, path: Path) -> None:
else:
self._init_library(path, open_status)

def _init_library(self, path: Path, open_status: LibraryStatus):
def _init_library(self, path: Path, open_status: LibraryStatus, is_test: bool = False):
# TODO: Don't have an is_test parameter, the frontend and backend tasks here can be split.
Comment thread
CyanVoxel marked this conversation as resolved.
if not open_status.success:
self.show_error_message(
error_name=open_status.message
Expand All @@ -1661,7 +1662,8 @@ def _init_library(self, path: Path, open_status: LibraryStatus):
return open_status

assert self.lib.library_dir
self.init_workers()
if not is_test:
self.init_workers()
Ignore.get_patterns(self.lib.library_dir, include_global=True)
self.__reset_navigation()

Expand Down
2 changes: 1 addition & 1 deletion tests/qt/test_file_path_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def test_title_update(
qt_driver.main_window.menu_bar.folders_to_tags_action = QAction(menu_bar)

# Trigger the update
qt_driver._init_library(library_dir, open_status)
qt_driver._init_library(library_dir, open_status, is_test=True)

# Assert the title is updated correctly
qt_driver.main_window.setWindowTitle.assert_called_with(expected_title(library_dir, base_title))
Loading