From 83a05b9041bfbe3f1aeccc13ce56c06a8c89ff17 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 14 Aug 2026 17:10:28 -0700 Subject: [PATCH 1/8] ADFA-5141: Pin doc DB page_size to 2048 in vacuum_database The 1024-byte page_size (SQLite's old default) carries ~2x the per-page header overhead of 2048 on the real ~300MB docdb, with no tradeoff versus larger sizes tested. vacuum_database() already rewrites the whole file on every delete-bearing mutation, so setting PRAGMA page_size before that VACUUM migrates the file for free on the next regeneration rather than as a one-off. Co-Authored-By: Claude Sonnet 5 --- docdb-studio/docdb_studio.py | 34 +++++++++++++++++++------------ docdb-studio/tests/test_vacuum.py | 11 ++++++++++ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index 7cce93c1..30b9c0cb 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -37,7 +37,9 @@ # builds cap at 999 host parameters, modern ones at 32766; we stay well under. SQL_PARAM_BATCH = 500 -PAGE_SIZE = 50 +SQLITE_PAGE_SIZE_BYTES = 2048 # ADFA-5141: smallest on the real ~300MB docdb, of the sizes tested. + +UI_PAGE_SIZE = 50 # Built-in HTTP server for browsing Content rows in a real browser. CONTENT_SERVER_PORT = 6175 @@ -677,6 +679,11 @@ def vacuum_database(db_path: Path) -> None: """Reclaim free pages by rewriting the DB file. Runs after every mutation that includes a DELETE — DB hygiene is non-negotiable per project policy. + Also pins the page size to SQLITE_PAGE_SIZE_BYTES (ADFA-5141): PRAGMA + page_size only takes effect on the following VACUUM, so it's set here + rather than at connect time. Once the file is at the target page size + this is a no-op. + VACUUM cannot run inside a transaction, so this opens a fresh connection with isolation_level=None and issues the statement directly. Callers should invoke this from a worker thread when triggered from the UI: on a ~380 MB @@ -685,6 +692,7 @@ def vacuum_database(db_path: Path) -> None: """ conn = sqlite3.connect(db_path, isolation_level=None) try: + conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") conn.execute("VACUUM") finally: conn.close() @@ -1645,7 +1653,7 @@ async def _on_window_event(e: "ft.WindowEvent") -> None: atexit.register(_send_app_closed) current_page = 1 - total_pages = max(1, (total_count + PAGE_SIZE - 1) // PAGE_SIZE) + total_pages = max(1, (total_count + UI_PAGE_SIZE - 1) // UI_PAGE_SIZE) # Built-in HTTP server for browsing Content rows in a real browser. # Bind failure (e.g. another instance already running) is non-fatal — @@ -3552,7 +3560,7 @@ def show_validate_uris_page(restore_page: int | None = None) -> None: ] total_pages_local = max( - 1, (len(issues) + PAGE_SIZE - 1) // PAGE_SIZE + 1, (len(issues) + UI_PAGE_SIZE - 1) // UI_PAGE_SIZE ) initial_page = ( max(1, min(restore_page, total_pages_local)) @@ -3576,8 +3584,8 @@ def show_validate_uris_page(restore_page: int | None = None) -> None: next_btn = ft.Button("Next") def fill_table() -> None: - offset = (validator_state["page"] - 1) * PAGE_SIZE - page_slice = issues[offset : offset + PAGE_SIZE] + offset = (validator_state["page"] - 1) * UI_PAGE_SIZE + page_slice = issues[offset : offset + UI_PAGE_SIZE] table.rows.clear() for tid, tag, problem in page_slice: edit_btn = ft.Button( @@ -3607,11 +3615,11 @@ def fill_table() -> None: def update_pager() -> None: start = ( - (validator_state["page"] - 1) * PAGE_SIZE + 1 + (validator_state["page"] - 1) * UI_PAGE_SIZE + 1 if issues else 0 ) - end = min(validator_state["page"] * PAGE_SIZE, len(issues)) + end = min(validator_state["page"] * UI_PAGE_SIZE, len(issues)) pager_text.value = ( f"Page {validator_state['page']} of {total_pages_local}" f" — issues {start}–{end} of {len(issues)}" @@ -3663,7 +3671,7 @@ def show_browse_page( ) -> None: nonlocal current_page, total_pages total_count = initial_total_count - total_pages = max(1, (total_count + PAGE_SIZE - 1) // PAGE_SIZE) + total_pages = max(1, (total_count + UI_PAGE_SIZE - 1) // UI_PAGE_SIZE) current_page = restore_page if restore_page is not None else 1 current_page = max(1, min(current_page, total_pages)) page.controls.clear() @@ -3688,7 +3696,7 @@ def run_search(_: ft.ControlEvent) -> None: "results_count": total_count, "is_wildcard": "*" in term, }) - total_pages = max(1, (total_count + PAGE_SIZE - 1) // PAGE_SIZE) + total_pages = max(1, (total_count + UI_PAGE_SIZE - 1) // UI_PAGE_SIZE) current_page = 1 fill_table() update_pager() @@ -3733,8 +3741,8 @@ def fill_table() -> None: category_column.fixed_width = category_column_width(db_path) term = (search_input.value or "").strip() search_term = term if term else None - offset = (current_page - 1) * PAGE_SIZE - rows = get_page(db_path, PAGE_SIZE, offset, search_term) + offset = (current_page - 1) * UI_PAGE_SIZE + rows = get_page(db_path, UI_PAGE_SIZE, offset, search_term) table.rows.clear() for i, row in enumerate(rows): tooltip_id, category_val, tag_val, summary_val, detail_val = row @@ -3817,8 +3825,8 @@ def go_next(_: ft.ControlEvent) -> None: pager_row = ft.Row(controls=[ft.Text(""), pager_text, ft.Text("")]) def update_pager() -> None: - start = (current_page - 1) * PAGE_SIZE + 1 if total_count > 0 else 0 - end = min(current_page * PAGE_SIZE, total_count) + start = (current_page - 1) * UI_PAGE_SIZE + 1 if total_count > 0 else 0 + end = min(current_page * UI_PAGE_SIZE, total_count) pager_text.value = f"Page {current_page} of {total_pages} — rows {start}–{end} of {total_count}" prev_btn.disabled = current_page <= 1 next_btn.disabled = current_page >= total_pages diff --git a/docdb-studio/tests/test_vacuum.py b/docdb-studio/tests/test_vacuum.py index 4aebeaa4..aa0ee00f 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -93,6 +93,17 @@ def test_vacuum_database_runs_without_error_and_preserves_schema() -> None: db.unlink(missing_ok=True) +def test_vacuum_database_sets_target_page_size() -> None: + db = _make_tooltip_db(seed_filler_rows=10) + try: + vacuum_database(db) + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + def test_vacuum_database_shrinks_after_large_delete() -> None: db = _make_tooltip_db(seed_filler_rows=200) try: From 97ff1a63da31a8a9f71ba29189eedd084e568af2 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 15 Aug 2026 15:24:09 -0700 Subject: [PATCH 2/8] ADFA-5141: Fix page_size migration gaps found in review - Trigger the migration on pure-insert paths too (import_content_files, import_csv_rows), not just delete/overwrite-gated ones, via a new _page_size_migration_pending check. - Work around WAL journal mode silently preventing PRAGMA page_size from taking effect on VACUUM. - Pin page_size on the release pipeline's DocumentationDatabase at creation time, so the actually-shipped DB gets the fix for free instead of only a maintainer's locally vacuumed copy. - Raise fetch_content_for_path's busy timeout so a concurrent VACUUM can't surface real content as a false 404. - Correct vacuum_database's docstring (VACUUM is never a no-op) and align its connection handling with the repo's with-block convention. - Update README's "no migrations" claim and add test coverage for the real 1024->2048 migration, WAL mode, and both pure-insert paths. Co-Authored-By: Claude Sonnet 5 --- docdb-studio/README.md | 2 +- docdb-studio/docdb_studio.py | 43 +++++++--- docdb-studio/tests/test_content_import.py | 8 +- docdb-studio/tests/test_vacuum.py | 96 +++++++++++++++++++++-- scripts/DocumentationDatabase.py | 7 ++ scripts/test_create_empty_database.py | 10 +++ 6 files changed, 150 insertions(+), 16 deletions(-) diff --git a/docdb-studio/README.md b/docdb-studio/README.md index 7d174003..13c2f4a9 100644 --- a/docdb-studio/README.md +++ b/docdb-studio/README.md @@ -200,4 +200,4 @@ docdb-studio/ └── pyproject.toml # project metadata + dependencies ``` -The database schema is documented in `SCHEMA.md` and is treated as immutable — the tool performs no migrations. +The database schema is documented in `SCHEMA.md` and is treated as immutable — the tool performs no schema migrations. (It does pin the on-disk SQLite page size to `SQLITE_PAGE_SIZE_BYTES` via `vacuum_database`, ADFA-5141 — a storage-format change, not a schema change.) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index 30b9c0cb..3a492198 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -681,8 +681,13 @@ def vacuum_database(db_path: Path) -> None: Also pins the page size to SQLITE_PAGE_SIZE_BYTES (ADFA-5141): PRAGMA page_size only takes effect on the following VACUUM, so it's set here - rather than at connect time. Once the file is at the target page size - this is a no-op. + rather than at connect time. VACUUM always does its full rebuild-and-copy + of the file regardless of whether the page size actually changes — there + is no cheaper no-op path once the DB is already at the target size. + + PRAGMA page_size silently has no effect on the following VACUUM when + journal_mode is WAL, so this temporarily switches to DELETE mode for the + rewrite and restores the original mode afterward. VACUUM cannot run inside a transaction, so this opens a fresh connection with isolation_level=None and issues the statement directly. Callers should @@ -690,12 +695,28 @@ def vacuum_database(db_path: Path) -> None: DB the rewrite takes several seconds and would otherwise freeze the event loop. """ - conn = sqlite3.connect(db_path, isolation_level=None) - try: + with sqlite3.connect(db_path, isolation_level=None) as conn: + (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() + if journal_mode.lower() == "wal": + conn.execute("PRAGMA journal_mode=DELETE") conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") conn.execute("VACUUM") - finally: - conn.close() + if journal_mode.lower() == "wal": + conn.execute(f"PRAGMA journal_mode={journal_mode}") + + +def _page_size_migration_pending(db_path: Path) -> bool: + """Cheap read-only check for whether vacuum_database still needs to run to + reach SQLITE_PAGE_SIZE_BYTES (ADFA-5141). + + vacuum_database is otherwise only triggered by DB-hygiene call sites gated + on "did this mutation delete/overwrite anything" — which a pure-insert + workflow never satisfies. Callers OR this into that gate so the one-time + migration still happens on the tool's common all-insert paths. + """ + with sqlite3.connect(db_path) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + return page_size != SQLITE_PAGE_SIZE_BYTES def get_categories_for_tooltips( @@ -766,7 +787,11 @@ def fetch_content_for_path( and decompressing brotli where applicable. None if the path is missing or decompression fails.""" try: - with sqlite3.connect(db_path) as conn: + # vacuum_database can hold an exclusive lock for several seconds on the + # ~380 MB DB (longer on the one-time ADFA-5141 page_size migration); + # the default 5s busy timeout would otherwise turn that into a false + # 404 for content that genuinely exists. + with sqlite3.connect(db_path, timeout=30.0) as conn: cur = conn.execute( """ SELECT c.content, ct.value, ct.compression @@ -1231,7 +1256,7 @@ def import_csv_rows( (tooltip_id, bid, label, uri), ) conn.commit() - if updated: + if updated or _page_size_migration_pending(db_path): vacuum_database(db_path) return inserted, updated @@ -1549,7 +1574,7 @@ def _report(phase: str, current: int, total: int) -> None: update_last_change(db_path, documentation_set, user_name) - if orphans_deleted or files_overwritten: + if orphans_deleted or files_overwritten or _page_size_migration_pending(db_path): _report("vacuum", 0, 1) vacuum_database(db_path) _report("vacuum", 1, 1) diff --git a/docdb-studio/tests/test_content_import.py b/docdb-studio/tests/test_content_import.py index 092288b2..89fb21ac 100644 --- a/docdb-studio/tests/test_content_import.py +++ b/docdb-studio/tests/test_content_import.py @@ -24,11 +24,17 @@ def _make_db_with_content_schema() -> Path: - """Create a temp DB seeded with Content/ContentTypes/Languages/LastChange schema and a few rows.""" + """Create a temp DB seeded with Content/ContentTypes/Languages/LastChange schema and a few rows. + + Pinned to SQLITE_PAGE_SIZE_BYTES up front so these tests (about import + behavior, not migration) don't incidentally trigger the one-time ADFA-5141 + page_size vacuum -- that path has its own dedicated tests in test_vacuum.py. + """ fd, path = tempfile.mkstemp(suffix=".db") Path(path).unlink(missing_ok=True) p = Path(path) with sqlite3.connect(p) as conn: + conn.execute(f"PRAGMA page_size={docdb_studio.SQLITE_PAGE_SIZE_BYTES}") conn.executescript( """ CREATE TABLE Languages (id INTEGER PRIMARY KEY, value TEXT NOT NULL UNIQUE); diff --git a/docdb-studio/tests/test_vacuum.py b/docdb-studio/tests/test_vacuum.py index aa0ee00f..95c0b5bd 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -21,12 +21,19 @@ ImportItem = docdb_studio.ImportItem -def _make_tooltip_db(seed_filler_rows: int = 0) -> Path: - """Tooltip-shaped temp DB with optional seed rows to inflate the file.""" +def _make_tooltip_db( + seed_filler_rows: int = 0, starting_page_size: int | None = None +) -> Path: + """Tooltip-shaped temp DB with optional seed rows to inflate the file. + + `starting_page_size`, when given, is set via PRAGMA before any table is + created — page_size only takes effect on an empty database.""" fd, path = tempfile.mkstemp(suffix=".db") Path(path).unlink(missing_ok=True) p = Path(path) with sqlite3.connect(p) as conn: + if starting_page_size is not None: + conn.execute(f"PRAGMA page_size={starting_page_size}") conn.executescript( """ CREATE TABLE TooltipCategories ( @@ -94,12 +101,34 @@ def test_vacuum_database_runs_without_error_and_preserves_schema() -> None: def test_vacuum_database_sets_target_page_size() -> None: - db = _make_tooltip_db(seed_filler_rows=10) + """Real production DBs start at page_size=1024 (ADFA-5141); exercise that + actual 1024 -> 2048 growth, not just an already-larger compiled default.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + (page_size_before,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size_before == 1024 + vacuum_database(db) + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + +def test_vacuum_database_migrates_page_size_under_wal() -> None: + """PRAGMA page_size silently fails to take effect on VACUUM under WAL + journal mode; vacuum_database must work around it.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") vacuum_database(db) with sqlite3.connect(db) as conn: (page_size,) = conn.execute("PRAGMA page_size").fetchone() + (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + assert journal_mode.lower() == "wal" finally: db.unlink(missing_ok=True) @@ -212,14 +241,34 @@ def test_import_csv_rows_update_mode_does_not_grow_file() -> None: db.unlink(missing_ok=True) +def test_import_csv_rows_pure_insert_still_migrates_page_size() -> None: + """A pure-insert import (no rows updated) must not skip the ADFA-5141 + page_size migration just because it never satisfies the DB-hygiene + delete/overwrite gate.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + rows = [ + ["ide", "brand-new", "summary", "detail", "", "", "", "", "", ""], + ] + inserted, updated = import_csv_rows(db, rows, {"ide": 1}, mode="insert") + assert (inserted, updated) == (1, 0) + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + # ---------- import_content_files (orphan delete) ---------- -def _make_content_db() -> Path: +def _make_content_db(starting_page_size: int | None = None) -> Path: fd, path = tempfile.mkstemp(suffix=".db") Path(path).unlink(missing_ok=True) p = Path(path) with sqlite3.connect(p) as conn: + if starting_page_size is not None: + conn.execute(f"PRAGMA page_size={starting_page_size}") conn.executescript( """ CREATE TABLE Languages (id INTEGER PRIMARY KEY, value TEXT NOT NULL UNIQUE); @@ -288,8 +337,45 @@ def test_import_content_files_orphan_phase_shrinks_file(tmp_path: Path) -> None: db.unlink(missing_ok=True) +def test_import_content_files_pure_insert_still_migrates_page_size( + tmp_path: Path, +) -> None: + """A pure-insert import (no orphans, no overwrites) must not skip the + ADFA-5141 page_size migration just because it never satisfies the + DB-hygiene delete/overwrite gate.""" + db = _make_content_db(starting_page_size=1024) + try: + new_file = tmp_path / "docs" / "new.html" + new_file.parent.mkdir() + new_file.write_bytes(b"hello") + content_types = get_content_types(db) + scan = prescan_content_import(db, tmp_path / "docs", content_types) + assert not scan.orphan_row_ids + assert not scan.overwrite_row_ids_by_base + + summary = import_content_files( + db, + scan.mapped, + language_id=1, + user_name="tester", + documentation_set="test", + orphan_row_ids=scan.orphan_row_ids, + overwrite_row_ids_by_base=scan.overwrite_row_ids_by_base, + progress_callback=None, + ) + assert summary.orphans_deleted == 0 + assert summary.files_overwritten == 0 + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + def test_import_content_files_vacuum_phase_is_reported() -> None: - """Pure-insert import skips VACUUM; delete-bearing import fires the phase.""" + """Delete-bearing import fires the vacuum phase (a pure-insert import also + fires it once, to migrate page_size -- see + test_import_content_files_pure_insert_still_migrates_page_size).""" db = _make_content_db() try: # Seed one row that the import will overwrite. diff --git a/scripts/DocumentationDatabase.py b/scripts/DocumentationDatabase.py index bd4ffef0..01db834b 100644 --- a/scripts/DocumentationDatabase.py +++ b/scripts/DocumentationDatabase.py @@ -10,6 +10,8 @@ import contextlib class DocumentationDatabase: + SQLITE_PAGE_SIZE_BYTES = 2048 # ADFA-5141: match docdb-studio's pinned page size + COMPRESSORS = { 'text': 'brotli', 'image': 'none', @@ -69,6 +71,11 @@ def __init__(self, database_path): if not os.path.exists(database_path) or os.path.getsize(database_path) == 0: with self.get_connection() as connection: cursor = connection.cursor() + # PRAGMA page_size only takes effect on an empty database, before + # the first table is created (ADFA-5141) -- this is the one place + # in the release pipeline where the DB is guaranteed empty, so + # pinning it here is free (no VACUUM rewrite needed). + connection.execute(f"PRAGMA page_size={self.SQLITE_PAGE_SIZE_BYTES}") self.create_tables(cursor) self.populate_content_types(cursor) self.populate_languages(cursor) diff --git a/scripts/test_create_empty_database.py b/scripts/test_create_empty_database.py index c8889576..ecd11b3f 100644 --- a/scripts/test_create_empty_database.py +++ b/scripts/test_create_empty_database.py @@ -27,6 +27,16 @@ def test_init_creates_database(self): self.assertIn(('ContentTypes',), tables) # ide_tooltip_table is optional and created by tooltip processing scripts + def test_page_size_pinned_on_fresh_database(self): + # ADFA-5141: a freshly created database should already be at the + # pinned page size -- no VACUUM migration needed for the release + # pipeline's actual build path. + with sqlite3.connect(self.temp_db_file.name) as connection: + cursor = connection.cursor() + cursor.execute("PRAGMA page_size;") + (page_size,) = cursor.fetchone() + self.assertEqual(page_size, DocumentationDatabase.SQLITE_PAGE_SIZE_BYTES) + def test_content_types_populated(self): # Check if the ContentTypes table contains all expected types with sqlite3.connect(self.temp_db_file.name) as connection: From 1018a574ce0f886e4fdbf919bb95b9ddb04c8294 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 13:48:16 -0700 Subject: [PATCH 3/8] ADFA-5141: Fix residual gaps from the second code review pass - vacuum_database: wrap the journal_mode switch/VACUUM/restore in try/finally so a lock error mid-VACUUM can't leave the DB stuck in DELETE journal mode forever; also dedupe the was-WAL check. - _page_size_migration_pending: match fetch_content_for_path's 30s busy timeout, and cache confirmed-migrated paths so it stops reopening a connection on every import call once the one-time migration is done. - Add a cross-file test asserting scripts/DocumentationDatabase.py and docdb-studio/docdb_studio.py agree on SQLITE_PAGE_SIZE_BYTES, since the two tools are separately deployed with no shared import. Co-Authored-By: Claude Sonnet 5 --- docdb-studio/docdb_studio.py | 37 ++++++++++---- docdb-studio/tests/test_vacuum.py | 73 +++++++++++++++++++++++++++ scripts/test_create_empty_database.py | 23 +++++++++ 3 files changed, 124 insertions(+), 9 deletions(-) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index 3a492198..78d2dee0 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -687,7 +687,8 @@ def vacuum_database(db_path: Path) -> None: PRAGMA page_size silently has no effect on the following VACUUM when journal_mode is WAL, so this temporarily switches to DELETE mode for the - rewrite and restores the original mode afterward. + rewrite and restores the original mode afterward — even if the rewrite + itself fails, so a lock error doesn't leave the DB permanently off WAL. VACUUM cannot run inside a transaction, so this opens a fresh connection with isolation_level=None and issues the statement directly. Callers should @@ -697,12 +698,21 @@ def vacuum_database(db_path: Path) -> None: """ with sqlite3.connect(db_path, isolation_level=None) as conn: (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() - if journal_mode.lower() == "wal": - conn.execute("PRAGMA journal_mode=DELETE") - conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") - conn.execute("VACUUM") - if journal_mode.lower() == "wal": - conn.execute(f"PRAGMA journal_mode={journal_mode}") + was_wal = journal_mode.lower() == "wal" + try: + if was_wal: + conn.execute("PRAGMA journal_mode=DELETE") + conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") + conn.execute("VACUUM") + finally: + if was_wal: + conn.execute(f"PRAGMA journal_mode={journal_mode}") + _page_size_confirmed.add(Path(db_path)) + + +# Paths already confirmed at SQLITE_PAGE_SIZE_BYTES, so _page_size_migration_pending +# can skip re-opening the DB on every import call once the one-time migration is done. +_page_size_confirmed: set[Path] = set() def _page_size_migration_pending(db_path: Path) -> bool: @@ -714,9 +724,18 @@ def _page_size_migration_pending(db_path: Path) -> bool: workflow never satisfies. Callers OR this into that gate so the one-time migration still happens on the tool's common all-insert paths. """ - with sqlite3.connect(db_path) as conn: + db_path = Path(db_path) + if db_path in _page_size_confirmed: + return False + # Matches fetch_content_for_path's timeout: a concurrent vacuum_database can + # hold an exclusive lock for several seconds, and the default 5s busy timeout + # would otherwise surface a spurious failure on an import that has already committed. + with sqlite3.connect(db_path, timeout=30.0) as conn: (page_size,) = conn.execute("PRAGMA page_size").fetchone() - return page_size != SQLITE_PAGE_SIZE_BYTES + pending = page_size != SQLITE_PAGE_SIZE_BYTES + if not pending: + _page_size_confirmed.add(db_path) + return pending def get_categories_for_tooltips( diff --git a/docdb-studio/tests/test_vacuum.py b/docdb-studio/tests/test_vacuum.py index 95c0b5bd..58243f34 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -7,6 +7,9 @@ import sqlite3 import tempfile from pathlib import Path +from unittest import mock + +import pytest import docdb_studio @@ -133,6 +136,54 @@ def test_vacuum_database_migrates_page_size_under_wal() -> None: db.unlink(missing_ok=True) +class _FailOnVacuumConn: + """Wraps a real sqlite3.Connection, raising on VACUUM. sqlite3.Connection + is a C type and refuses attribute assignment (even per-instance), so this + proxies everything else through to a genuine connection instead.""" + + def __init__(self, real): + self._real = real + + def execute(self, sql, *args, **kwargs): + if sql.strip().upper() == "VACUUM": + raise sqlite3.OperationalError("simulated lock failure") + return self._real.execute(sql, *args, **kwargs) + + def __enter__(self): + self._real.__enter__() + return self + + def __exit__(self, *exc_info): + return self._real.__exit__(*exc_info) + + def __getattr__(self, name): + return getattr(self._real, name) + + +def test_vacuum_database_restores_wal_even_if_vacuum_fails() -> None: + """If VACUUM itself raises after journal_mode was switched away from WAL, + the original journal_mode must still be restored -- a lock error mid-vacuum + must not leave the DB stuck on DELETE journal mode forever.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + + real_connect = sqlite3.connect + with mock.patch("docdb_studio.sqlite3.connect") as mock_connect: + mock_connect.side_effect = lambda *a, **k: _FailOnVacuumConn( + real_connect(*a, **k) + ) + with pytest.raises(sqlite3.OperationalError): + vacuum_database(db) + + with sqlite3.connect(db) as conn: + (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() + assert journal_mode.lower() == "wal" + finally: + db.unlink(missing_ok=True) + + def test_vacuum_database_shrinks_after_large_delete() -> None: db = _make_tooltip_db(seed_filler_rows=200) try: @@ -259,6 +310,28 @@ def test_import_csv_rows_pure_insert_still_migrates_page_size() -> None: db.unlink(missing_ok=True) +def test_page_size_migration_pending_is_cached_once_confirmed() -> None: + """Once a DB is confirmed at SQLITE_PAGE_SIZE_BYTES, _page_size_migration_pending + must not keep reopening a connection to re-check on every call -- that would + add overhead (and busy-timeout risk) to every import forever, long after the + one-time migration is done.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + docdb_studio._page_size_confirmed.discard(Path(db)) + assert docdb_studio._page_size_migration_pending(db) is True + vacuum_database(db) + assert Path(db) in docdb_studio._page_size_confirmed + + real_connect = sqlite3.connect + with mock.patch("docdb_studio.sqlite3.connect") as mock_connect: + mock_connect.side_effect = real_connect + assert docdb_studio._page_size_migration_pending(db) is False + mock_connect.assert_not_called() + finally: + docdb_studio._page_size_confirmed.discard(Path(db)) + db.unlink(missing_ok=True) + + # ---------- import_content_files (orphan delete) ---------- diff --git a/scripts/test_create_empty_database.py b/scripts/test_create_empty_database.py index ecd11b3f..1b34d8cc 100644 --- a/scripts/test_create_empty_database.py +++ b/scripts/test_create_empty_database.py @@ -1,7 +1,9 @@ import unittest import os +import re import tempfile import sqlite3 +from pathlib import Path from DocumentationDatabase import DocumentationDatabase class TestDocumentationDatabase(unittest.TestCase): @@ -37,6 +39,27 @@ def test_page_size_pinned_on_fresh_database(self): (page_size,) = cursor.fetchone() self.assertEqual(page_size, DocumentationDatabase.SQLITE_PAGE_SIZE_BYTES) + def test_page_size_matches_docdb_studio(self): + # ADFA-5141: scripts/ and docdb-studio/ are separately deployed tools + # (own dependency files, no shared import) that each hardcode + # SQLITE_PAGE_SIZE_BYTES. Parse docdb_studio.py's source instead of + # importing it, since scripts/ doesn't have docdb-studio's + # dependencies (flet, etc.) installed -- this only guards against the + # two constants drifting apart, without requiring either tool's + # packaging to change. + docdb_studio_source = ( + Path(__file__).resolve().parents[1] / "docdb-studio" / "docdb_studio.py" + ).read_text() + match = re.search( + r"^SQLITE_PAGE_SIZE_BYTES\s*=\s*(\d+)", docdb_studio_source, re.MULTILINE + ) + self.assertIsNotNone( + match, "docdb-studio/docdb_studio.py must define SQLITE_PAGE_SIZE_BYTES" + ) + self.assertEqual( + DocumentationDatabase.SQLITE_PAGE_SIZE_BYTES, int(match.group(1)) + ) + def test_content_types_populated(self): # Check if the ContentTypes table contains all expected types with sqlite3.connect(self.temp_db_file.name) as connection: From 7bed79297af095e8ea702118683792fbaf6b6c50 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 15:51:23 -0700 Subject: [PATCH 4/8] ADFA-5141: Rewrite vacuum_database on VACUUM INTO to fix WAL deadlock The in-place VACUUM + journal_mode round-trip required exclusive access to db_path: SQLite refuses to switch a WAL-mode database away from WAL while ANY other connection has it open. Empirically reproduced this as a deterministic deadlock in import_csv_rows/import_content_files et al, and confirmed it goes deeper than "close the caller's own connection" -- an already-returned function's unclosed `with sqlite3.connect(...) as conn:` block can keep the file locked well past its own return, and that convention is used 30+ times in this file. A live UI connection left open elsewhere (e.g. a data browser) would hit the same deadlock. vacuum_database now rewrites into a temp file via VACUUM INTO, which only needs a read snapshot of the source, and atomically swaps it into place with os.replace -- this works regardless of what else has db_path open. journal_mode=WAL is reapplied to the new file's final path (VACUUM INTO always produces a plain rollback-journal file), and stale sidecars from the replaced file are cleaned up. Also, from the second review pass: - vacuum_database's connect uses the 30s timeout matching the other ADFA-5141 call sites. - delete_tooltips_bulk now also triggers on _page_size_migration_pending, not just `deleted` -- it had the same gap the import paths were already fixed for. - _page_size_confirmed is now lock-guarded (imports can run migration checks from worker threads). - Callers close their own connections before any vacuum-triggering call, matching the (no longer strictly required, but still good practice) discipline established while chasing the deadlock down. New tests cover: the WAL deadlock is gone even with an unrelated open connection AND an unclosed caller-style connection present at once, the original file is left untouched if VACUUM INTO fails partway (temp file cleaned up, no partial swap), and the migration-pending gate on each previously-gapped call path. Co-Authored-By: Claude Sonnet 5 --- docdb-studio/docdb_studio.py | 110 +++++++++++----- docdb-studio/tests/test_vacuum.py | 206 ++++++++++++++++++++++++++++-- 2 files changed, 273 insertions(+), 43 deletions(-) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index 78d2dee0..6a5834ea 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -11,6 +11,7 @@ import platform as _platform import sqlite3 import sys +import tempfile import threading import time import unicodedata @@ -673,46 +674,78 @@ def _stamp(conn: sqlite3.Connection, doc_set: str) -> None: if documentation_set != WHOLEDB_KEY: _stamp(conn, WHOLEDB_KEY) conn.commit() + conn.close() def vacuum_database(db_path: Path) -> None: - """Reclaim free pages by rewriting the DB file. Runs after every mutation - that includes a DELETE — DB hygiene is non-negotiable per project policy. - - Also pins the page size to SQLITE_PAGE_SIZE_BYTES (ADFA-5141): PRAGMA - page_size only takes effect on the following VACUUM, so it's set here - rather than at connect time. VACUUM always does its full rebuild-and-copy - of the file regardless of whether the page size actually changes — there - is no cheaper no-op path once the DB is already at the target size. - - PRAGMA page_size silently has no effect on the following VACUUM when - journal_mode is WAL, so this temporarily switches to DELETE mode for the - rewrite and restores the original mode afterward — even if the rewrite - itself fails, so a lock error doesn't leave the DB permanently off WAL. - - VACUUM cannot run inside a transaction, so this opens a fresh connection - with isolation_level=None and issues the statement directly. Callers should - invoke this from a worker thread when triggered from the UI: on a ~380 MB - DB the rewrite takes several seconds and would otherwise freeze the event - loop. + """Reclaim free pages and pin the page size (ADFA-5141) by rewriting the DB + into a fresh file via VACUUM INTO, then atomically swapping it into place. + Runs after every mutation that includes a DELETE — DB hygiene is + non-negotiable per project policy. + + This deliberately avoids in-place VACUUM + a journal_mode round-trip. + Switching a WAL-mode database away from WAL requires exclusive access -- + no other connection may have the file open at all -- which is impractical + to guarantee in a desktop app that may have other connections open + elsewhere (e.g. a live data browser, or even the caller's own connection: + Python's `with sqlite3.connect(...) as conn:` does not close conn on + exit, and empirically an unclosed connection can keep the file locked + well past its enclosing function's return). VACUUM INTO only needs a read + snapshot of the source, so it works regardless of what else currently has + db_path open. + + The rewrite happens in a temp file created next to db_path (so the final + os.replace is same-filesystem and atomic, and per the ADFA-5088 CWE-377 + lesson this avoids writing into a shared, guessable /tmp). VACUUM INTO + always produces a plain rollback-journal file regardless of the source's + journal_mode, so if the source was WAL, journal_mode=WAL is reapplied to + the new file (via its final path, so the resulting -wal/-shm sidecars get + the right name) before it replaces the original; any sidecars left behind + by the file just replaced are then stale and removed. + + Callers should invoke this from a worker thread when triggered from the + UI: on a ~380 MB DB the rewrite takes several seconds and would otherwise + freeze the event loop. """ - with sqlite3.connect(db_path, isolation_level=None) as conn: + db_path = Path(db_path) + with sqlite3.connect(db_path, timeout=30.0) as conn: (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() was_wal = journal_mode.lower() == "wal" - try: - if was_wal: - conn.execute("PRAGMA journal_mode=DELETE") + + fd, tmp_name = tempfile.mkstemp(dir=db_path.parent, suffix=".vacuum.tmp") + os.close(fd) + tmp_path = Path(tmp_name) + try: + with sqlite3.connect(db_path, timeout=30.0) as conn: conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") - conn.execute("VACUUM") - finally: - if was_wal: - conn.execute(f"PRAGMA journal_mode={journal_mode}") - _page_size_confirmed.add(Path(db_path)) + conn.execute(f"VACUUM INTO '{tmp_path}'") + os.replace(tmp_path, db_path) + finally: + tmp_path.unlink(missing_ok=True) + + # Any -wal/-shm sidecars still sitting at db_path's name at this point are + # for the file just replaced -- guaranteed stale, since the swapped-in + # file was just VACUUM INTO'd fresh (plain rollback-journal, no sidecars). + for suffix in ("-wal", "-shm"): + stale = db_path.with_name(db_path.name + suffix) + stale.unlink(missing_ok=True) + + if was_wal: + # Reapply on db_path's final name (not tmp_path's) so the resulting + # sidecars are named correctly -- VACUUM INTO always produces a plain + # rollback-journal file regardless of the source's journal_mode. + with sqlite3.connect(db_path) as conn: + conn.execute("PRAGMA journal_mode=WAL") + + with _page_size_confirmed_lock: + _page_size_confirmed.add(db_path) # Paths already confirmed at SQLITE_PAGE_SIZE_BYTES, so _page_size_migration_pending # can skip re-opening the DB on every import call once the one-time migration is done. +# Guarded by a lock since imports can run migration checks from worker threads. _page_size_confirmed: set[Path] = set() +_page_size_confirmed_lock = threading.Lock() def _page_size_migration_pending(db_path: Path) -> bool: @@ -725,16 +758,19 @@ def _page_size_migration_pending(db_path: Path) -> bool: migration still happens on the tool's common all-insert paths. """ db_path = Path(db_path) - if db_path in _page_size_confirmed: - return False + with _page_size_confirmed_lock: + if db_path in _page_size_confirmed: + return False # Matches fetch_content_for_path's timeout: a concurrent vacuum_database can # hold an exclusive lock for several seconds, and the default 5s busy timeout # would otherwise surface a spurious failure on an import that has already committed. with sqlite3.connect(db_path, timeout=30.0) as conn: (page_size,) = conn.execute("PRAGMA page_size").fetchone() + conn.close() pending = page_size != SQLITE_PAGE_SIZE_BYTES if not pending: - _page_size_confirmed.add(db_path) + with _page_size_confirmed_lock: + _page_size_confirmed.add(db_path) return pending @@ -781,7 +817,8 @@ def delete_tooltips_bulk(db_path: Path, tooltip_ids: list[int]) -> int: ) deleted += cur.rowcount conn.commit() - if deleted: + conn.close() + if deleted or _page_size_migration_pending(db_path): vacuum_database(db_path) return deleted @@ -1013,6 +1050,7 @@ def delete_tooltip_button( (tooltip_id, button_number_id), ) conn.commit() + conn.close() vacuum_database(db_path) @@ -1045,6 +1083,7 @@ def replace_tooltip_buttons( (tooltip_id, button_number_id, description, uri), ) conn.commit() + conn.close() vacuum_database(db_path) @@ -1275,6 +1314,7 @@ def import_csv_rows( (tooltip_id, bid, label, uri), ) conn.commit() + conn.close() if updated or _page_size_migration_pending(db_path): vacuum_database(db_path) return inserted, updated @@ -1344,7 +1384,9 @@ def get_content_types(db_path: Path) -> dict[str, tuple[int, str]]: """Return {ContentTypes.value: (id, compression)} for every row.""" with sqlite3.connect(db_path) as conn: cur = conn.execute("SELECT id, value, compression FROM ContentTypes") - return {value: (cid, compression) for cid, value, compression in cur.fetchall()} + result = {value: (cid, compression) for cid, value, compression in cur.fetchall()} + conn.close() + return result def get_languages(db_path: Path) -> list[tuple[int, str]]: @@ -1472,6 +1514,7 @@ def prescan_content_import( else: orphan_set.add(base) orphan_row_ids.append(row_id) + conn.close() return ScanResult( mapped=list(candidates), @@ -1590,6 +1633,7 @@ def _report(phase: str, current: int, total: int) -> None: _report("add", adds_done, add_total) conn.commit() + conn.close() update_last_change(db_path, documentation_set, user_name) diff --git a/docdb-studio/tests/test_vacuum.py b/docdb-studio/tests/test_vacuum.py index 58243f34..30ae0bb6 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -136,8 +136,53 @@ def test_vacuum_database_migrates_page_size_under_wal() -> None: db.unlink(missing_ok=True) -class _FailOnVacuumConn: - """Wraps a real sqlite3.Connection, raising on VACUUM. sqlite3.Connection +def test_vacuum_database_succeeds_with_other_connections_still_open() -> None: + """The in-place VACUUM + journal_mode round-trip this replaced required + exclusive access to db_path -- SQLite refuses to switch a WAL-mode db away + from WAL while any other connection has it open. That made vacuum_database + fragile against anything else in the app holding db_path open (e.g. a live + data browser), not just the caller's own unclosed connection. VACUUM INTO + only needs a read snapshot of the source, so this must succeed even with + both an unrelated open connection and an unclosed caller-style connection + still around.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + + # An unrelated, still-open connection with a live read outstanding -- + # e.g. a UI data browser left open while an import runs elsewhere. + browser_conn = sqlite3.connect(db) + browser_conn.execute("SELECT * FROM Tooltips") + + # A second connection that wrote+committed and is deliberately left + # unclosed, mirroring a caller that didn't close its own connection. + writer_conn = sqlite3.connect(db) + writer_conn.execute( + "INSERT INTO Tooltips (categoryId, tag, summary, detail) VALUES (?, ?, ?, ?)", + (1, "extra", "s", "d"), + ) + writer_conn.commit() + + try: + vacuum_database(db) # must not raise "database is locked" + finally: + browser_conn.close() + writer_conn.close() + + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() + (count,) = conn.execute("SELECT count(*) FROM Tooltips").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + assert journal_mode.lower() == "wal" + assert count == 11 # 10 seeded + 1 from writer_conn + finally: + db.unlink(missing_ok=True) + + +class _FailOnVacuumIntoConn: + """Wraps a real sqlite3.Connection, raising on VACUUM INTO. sqlite3.Connection is a C type and refuses attribute assignment (even per-instance), so this proxies everything else through to a genuine connection instead.""" @@ -145,8 +190,8 @@ def __init__(self, real): self._real = real def execute(self, sql, *args, **kwargs): - if sql.strip().upper() == "VACUUM": - raise sqlite3.OperationalError("simulated lock failure") + if sql.strip().upper().startswith("VACUUM INTO"): + raise sqlite3.OperationalError("simulated vacuum-into failure") return self._real.execute(sql, *args, **kwargs) def __enter__(self): @@ -160,26 +205,33 @@ def __getattr__(self, name): return getattr(self._real, name) -def test_vacuum_database_restores_wal_even_if_vacuum_fails() -> None: - """If VACUUM itself raises after journal_mode was switched away from WAL, - the original journal_mode must still be restored -- a lock error mid-vacuum - must not leave the DB stuck on DELETE journal mode forever.""" +def test_vacuum_database_leaves_original_untouched_if_vacuum_into_fails() -> None: + """If VACUUM INTO fails partway, the original db_path must be left + completely unchanged (content, page_size, journal_mode) -- vacuum_database + only ever swaps in a fully-built replacement, never edits db_path in place + -- and the temp file must not be left behind.""" db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) try: with sqlite3.connect(db) as conn: conn.execute("PRAGMA journal_mode=WAL") + size_before = db.stat().st_size real_connect = sqlite3.connect with mock.patch("docdb_studio.sqlite3.connect") as mock_connect: - mock_connect.side_effect = lambda *a, **k: _FailOnVacuumConn( + mock_connect.side_effect = lambda *a, **k: _FailOnVacuumIntoConn( real_connect(*a, **k) ) - with pytest.raises(sqlite3.OperationalError): + with pytest.raises(sqlite3.OperationalError, match="simulated vacuum-into failure"): vacuum_database(db) + assert db.stat().st_size == size_before with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() + assert page_size == 1024 assert journal_mode.lower() == "wal" + leftover_tmp = list(db.parent.glob(f"{db.name}*.vacuum.tmp")) + assert leftover_tmp == [] finally: db.unlink(missing_ok=True) @@ -216,6 +268,39 @@ def test_delete_tooltips_bulk_shrinks_file() -> None: db.unlink(missing_ok=True) +def test_delete_tooltips_bulk_no_op_still_migrates_page_size() -> None: + """A bulk-delete call where nothing actually matched (deleted == 0) must not + skip the ADFA-5141 page_size migration just because it never satisfies the + `if deleted:` DB-hygiene gate -- the same gap fixed for the import paths.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + deleted = delete_tooltips_bulk(db, [999999]) + assert deleted == 0 + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + +def test_delete_tooltips_bulk_under_wal_does_not_deadlock() -> None: + """SQLite refuses to switch a WAL-mode db away from WAL while any other + connection (even one that already committed) is still open; delete_tooltips_bulk + must close its own connection before the triggered vacuum_database runs.""" + db = _make_tooltip_db(seed_filler_rows=10) + try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + ids = [ + row[0] for row in conn.execute("SELECT id FROM Tooltips").fetchall() + ] + conn.close() + deleted = delete_tooltips_bulk(db, ids[:5]) + assert deleted == 5 + finally: + db.unlink(missing_ok=True) + + # ---------- delete_tooltip_button ---------- @@ -240,6 +325,30 @@ def test_delete_tooltip_button_does_not_grow_file() -> None: db.unlink(missing_ok=True) +def test_delete_tooltip_button_under_wal_does_not_deadlock() -> None: + """SQLite refuses to switch a WAL-mode db away from WAL while any other + connection (even one that already committed) is still open; + delete_tooltip_button must close its own connection before the + unconditionally-triggered vacuum_database runs.""" + db = _make_tooltip_db(seed_filler_rows=10) + try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + conn.execute( + "INSERT INTO Tooltips (id, categoryId, tag, summary, detail) VALUES (?, ?, ?, ?, ?)", + (9000, 1, "with-button", "s", "d"), + ) + conn.execute( + "INSERT INTO TooltipButtons (tooltipId, buttonNumberId, description, uri) VALUES (?, ?, ?, ?)", + (9000, 1, "desc", "uri.html"), + ) + conn.commit() + conn.close() + delete_tooltip_button(db, 9000, 1) # must not raise + finally: + db.unlink(missing_ok=True) + + # ---------- replace_tooltip_buttons ---------- @@ -266,6 +375,26 @@ def test_replace_tooltip_buttons_does_not_grow_file() -> None: db.unlink(missing_ok=True) +def test_replace_tooltip_buttons_under_wal_does_not_deadlock() -> None: + """SQLite refuses to switch a WAL-mode db away from WAL while any other + connection (even one that already committed) is still open; + replace_tooltip_buttons must close its own connection before the + unconditionally-triggered vacuum_database runs.""" + db = _make_tooltip_db(seed_filler_rows=10) + try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + conn.execute( + "INSERT INTO Tooltips (id, categoryId, tag, summary, detail) VALUES (?, ?, ?, ?, ?)", + (9001, 1, "replace-me", "s", "d"), + ) + conn.commit() + conn.close() + replace_tooltip_buttons(db, 9001, [(1, "new1", "new1.html")]) # must not raise + finally: + db.unlink(missing_ok=True) + + # ---------- import_csv_rows (update mode) ---------- @@ -310,6 +439,27 @@ def test_import_csv_rows_pure_insert_still_migrates_page_size() -> None: db.unlink(missing_ok=True) +def test_import_csv_rows_pure_insert_migrates_page_size_under_wal() -> None: + """The migration-triggered vacuum_database must not deadlock: SQLite refuses + to switch a WAL-mode db away from WAL while any other connection (even one + that already committed) is still open, so every mutating helper that can + trigger vacuum_database must close its own connection first.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + rows = [ + ["ide", "brand-new", "summary", "detail", "", "", "", "", "", ""], + ] + inserted, updated = import_csv_rows(db, rows, {"ide": 1}, mode="insert") + assert (inserted, updated) == (1, 0) + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + def test_page_size_migration_pending_is_cached_once_confirmed() -> None: """Once a DB is confirmed at SQLITE_PAGE_SIZE_BYTES, _page_size_migration_pending must not keep reopening a connection to re-check on every call -- that would @@ -445,6 +595,42 @@ def test_import_content_files_pure_insert_still_migrates_page_size( db.unlink(missing_ok=True) +def test_import_content_files_pure_insert_migrates_page_size_under_wal( + tmp_path: Path, +) -> None: + """SQLite refuses to switch a WAL-mode db away from WAL while any other + connection (even one that already committed) is still open; + import_content_files must close its own connection before the migration + -triggered vacuum_database runs.""" + db = _make_content_db(starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + conn.execute("PRAGMA journal_mode=WAL") + conn.close() + new_file = tmp_path / "docs" / "new.html" + new_file.parent.mkdir() + new_file.write_bytes(b"hello") + content_types = get_content_types(db) + scan = prescan_content_import(db, tmp_path / "docs", content_types) + + summary = import_content_files( + db, + scan.mapped, + language_id=1, + user_name="tester", + documentation_set="test", + orphan_row_ids=scan.orphan_row_ids, + overwrite_row_ids_by_base=scan.overwrite_row_ids_by_base, + progress_callback=None, + ) + assert summary.files_imported == 1 + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + def test_import_content_files_vacuum_phase_is_reported() -> None: """Delete-bearing import fires the vacuum phase (a pure-insert import also fires it once, to migrate page_size -- see From 8022ab5f0576f8a9f3f34ae0f8f4045bd2e6f11b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 16:40:58 -0700 Subject: [PATCH 5/8] ADFA-5141: Restore file permissions after the VACUUM INTO swap tempfile.mkstemp() always creates its file mode 0600 regardless of the original's mode or the process umask. The VACUUM INTO rewrite swaps that temp file into db_path's place via os.replace, which never restored the original permissions -- alexmmiller's QA on real hardware caught this silently dropping documentation.db from 644 to 600 on every vacuum. Capture db_path's mode before the rewrite and os.chmod it back after the swap. New test confirms a 644 file stays 644 across vacuum_database (and fails against the pre-fix code, dropping to 600). Co-Authored-By: Claude Sonnet 5 --- docdb-studio/docdb_studio.py | 11 +++++++++-- docdb-studio/tests/test_vacuum.py | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index 6a5834ea..dec4805a 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -10,6 +10,7 @@ import os import platform as _platform import sqlite3 +import stat import sys import tempfile import threading @@ -696,8 +697,12 @@ def vacuum_database(db_path: Path) -> None: The rewrite happens in a temp file created next to db_path (so the final os.replace is same-filesystem and atomic, and per the ADFA-5088 CWE-377 - lesson this avoids writing into a shared, guessable /tmp). VACUUM INTO - always produces a plain rollback-journal file regardless of the source's + lesson this avoids writing into a shared, guessable /tmp). tempfile.mkstemp + always creates its file mode 0600 regardless of the original's mode or the + process umask, so db_path's original permission bits are restored on the + swapped-in file (confirmed on real hardware during QA: without this, every + vacuum silently dropped a 644 documentation.db to 600). VACUUM INTO always + produces a plain rollback-journal file regardless of the source's journal_mode, so if the source was WAL, journal_mode=WAL is reapplied to the new file (via its final path, so the resulting -wal/-shm sidecars get the right name) before it replaces the original; any sidecars left behind @@ -708,6 +713,7 @@ def vacuum_database(db_path: Path) -> None: freeze the event loop. """ db_path = Path(db_path) + original_mode = stat.S_IMODE(db_path.stat().st_mode) with sqlite3.connect(db_path, timeout=30.0) as conn: (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() was_wal = journal_mode.lower() == "wal" @@ -720,6 +726,7 @@ def vacuum_database(db_path: Path) -> None: conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") conn.execute(f"VACUUM INTO '{tmp_path}'") os.replace(tmp_path, db_path) + os.chmod(db_path, original_mode) finally: tmp_path.unlink(missing_ok=True) diff --git a/docdb-studio/tests/test_vacuum.py b/docdb-studio/tests/test_vacuum.py index 30ae0bb6..8e5da1f6 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -4,7 +4,9 @@ so freed pages are reclaimed immediately. These tests cover the helper itself and one end-to-end shrinkage check per delete-bearing helper.""" +import os import sqlite3 +import stat import tempfile from pathlib import Path from unittest import mock @@ -136,6 +138,21 @@ def test_vacuum_database_migrates_page_size_under_wal() -> None: db.unlink(missing_ok=True) +def test_vacuum_database_preserves_file_permissions() -> None: + """VACUUM INTO rewrites through a tempfile.mkstemp() temp file, which is + always created mode 0600 regardless of the original's mode or the process + umask -- confirmed via real-world QA to silently drop a 644 documentation.db + to 600 on every vacuum if not restored after the os.replace swap.""" + db = _make_tooltip_db(seed_filler_rows=10) + try: + os.chmod(db, 0o644) + vacuum_database(db) + mode = stat.S_IMODE(db.stat().st_mode) + assert mode == 0o644 + finally: + db.unlink(missing_ok=True) + + def test_vacuum_database_succeeds_with_other_connections_still_open() -> None: """The in-place VACUUM + journal_mode round-trip this replaced required exclusive access to db_path -- SQLite refuses to switch a WAL-mode db away From 8a6039931e4afb84d5fa842d4c0c00f9e69de79b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 16:52:30 -0700 Subject: [PATCH 6/8] ADFA-5141: Fix findings from a second self-review of the VACUUM INTO rewrite - Escape embedded single quotes in the VACUUM INTO target path. It's a SQL string literal, not a bindable parameter, so a db_path whose parent directory contains a quote (a real user directory name like "David's Docs") broke the statement outright -- confirmed empirically. - Serialize vacuum_database via a module-level lock. VACUUM INTO takes a read snapshot; without this, two concurrent invocations on the same db_path could race, with the second's os.replace silently discarding a write the first's snapshot missed. This only serializes vacuum_database against itself -- see its docstring for the residual gap against a fully independent concurrent writer. - Close the journal_mode-read connection explicitly instead of relying on it being reassigned by the next `with` block, matching every other connection touched by this fix. - insert_tooltip/update_tooltip/add_tooltip_button/update_tooltip_button never called vacuum_database at all (no delete, so the DB-hygiene gate never fired), so a docdb edited only through those single-row paths never migrated to SQLITE_PAGE_SIZE_BYTES. Added the same _page_size_migration_pending gate used elsewhere -- cheap once cached, so this only costs a real VACUUM the one time it's needed. New tests: quote-in-path handling, concurrent vacuum_database calls don't raise or corrupt the file, and each of the four previously-gapped single-row mutation functions now migrates page_size. Co-Authored-By: Claude Sonnet 5 --- docdb-studio/docdb_studio.py | 104 ++++++++++++++++++-------- docdb-studio/tests/test_vacuum.py | 120 ++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 31 deletions(-) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index dec4805a..844ec471 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -678,12 +678,22 @@ def _stamp(conn: sqlite3.Connection, doc_set: str) -> None: conn.close() +# Serializes vacuum_database so two concurrent callers can't race: VACUUM +# INTO takes a read snapshot, and without this a write committed by another +# connection between one caller's snapshot and its later os.replace would be +# silently dropped when that stale snapshot gets swapped into place. +_vacuum_lock = threading.Lock() + + def vacuum_database(db_path: Path) -> None: """Reclaim free pages and pin the page size (ADFA-5141) by rewriting the DB into a fresh file via VACUUM INTO, then atomically swapping it into place. Runs after every mutation that includes a DELETE — DB hygiene is non-negotiable per project policy. + Serialized process-wide via _vacuum_lock: without it, two concurrent + callers could race (see _vacuum_lock's own comment for the failure mode). + This deliberately avoids in-place VACUUM + a journal_mode round-trip. Switching a WAL-mode database away from WAL requires exclusive access -- no other connection may have the file open at all -- which is impractical @@ -711,38 +721,57 @@ def vacuum_database(db_path: Path) -> None: Callers should invoke this from a worker thread when triggered from the UI: on a ~380 MB DB the rewrite takes several seconds and would otherwise freeze the event loop. - """ - db_path = Path(db_path) - original_mode = stat.S_IMODE(db_path.stat().st_mode) - with sqlite3.connect(db_path, timeout=30.0) as conn: - (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() - was_wal = journal_mode.lower() == "wal" - fd, tmp_name = tempfile.mkstemp(dir=db_path.parent, suffix=".vacuum.tmp") - os.close(fd) - tmp_path = Path(tmp_name) - try: + Note: this only serializes vacuum_database against itself -- it does not + protect against a plain write (one that never reaches this function, e.g. + a concurrent insert/update on another thread outside the DB-hygiene gate) + committing during the snapshot-to-swap window and being silently lost. + Closing that gap fully would mean every writer in this file taking + _vacuum_lock around its own write+commit too; not done here since it + depends on this app's broader UI threading model (whether the UI actually + permits overlapping mutations on one db_path) rather than anything local + to this function. + """ + with _vacuum_lock: + db_path = Path(db_path) + original_mode = stat.S_IMODE(db_path.stat().st_mode) with sqlite3.connect(db_path, timeout=30.0) as conn: - conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") - conn.execute(f"VACUUM INTO '{tmp_path}'") - os.replace(tmp_path, db_path) - os.chmod(db_path, original_mode) - finally: - tmp_path.unlink(missing_ok=True) - - # Any -wal/-shm sidecars still sitting at db_path's name at this point are - # for the file just replaced -- guaranteed stale, since the swapped-in - # file was just VACUUM INTO'd fresh (plain rollback-journal, no sidecars). - for suffix in ("-wal", "-shm"): - stale = db_path.with_name(db_path.name + suffix) - stale.unlink(missing_ok=True) - - if was_wal: - # Reapply on db_path's final name (not tmp_path's) so the resulting - # sidecars are named correctly -- VACUUM INTO always produces a plain - # rollback-journal file regardless of the source's journal_mode. - with sqlite3.connect(db_path) as conn: - conn.execute("PRAGMA journal_mode=WAL") + (journal_mode,) = conn.execute("PRAGMA journal_mode").fetchone() + was_wal = journal_mode.lower() == "wal" + conn.close() + + fd, tmp_name = tempfile.mkstemp(dir=db_path.parent, suffix=".vacuum.tmp") + os.close(fd) + tmp_path = Path(tmp_name) + # SQLite string-literal escaping (double any embedded single quote) -- + # VACUUM INTO takes its target as a string literal, not a bindable + # parameter, so a path containing a quote (e.g. a user's home directory + # named "David's Docs") would otherwise break the statement. + escaped_tmp_path = str(tmp_path).replace("'", "''") + try: + with sqlite3.connect(db_path, timeout=30.0) as conn: + conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") + conn.execute(f"VACUUM INTO '{escaped_tmp_path}'") + os.replace(tmp_path, db_path) + os.chmod(db_path, original_mode) + finally: + tmp_path.unlink(missing_ok=True) + + # Any -wal/-shm sidecars still sitting at db_path's name at this point + # are for the file just replaced -- guaranteed stale, since the + # swapped-in file was just VACUUM INTO'd fresh (plain rollback-journal, + # no sidecars). + for suffix in ("-wal", "-shm"): + stale = db_path.with_name(db_path.name + suffix) + stale.unlink(missing_ok=True) + + if was_wal: + # Reapply on db_path's final name (not tmp_path's) so the + # resulting sidecars are named correctly -- VACUUM INTO always + # produces a plain rollback-journal file regardless of the + # source's journal_mode. + with sqlite3.connect(db_path) as conn: + conn.execute("PRAGMA journal_mode=WAL") with _page_size_confirmed_lock: _page_size_confirmed.add(db_path) @@ -967,7 +996,11 @@ def insert_tooltip( (category_id, tag, summary, detail), ) conn.commit() - return cur.lastrowid or 0 + new_id = cur.lastrowid or 0 + conn.close() + if _page_size_migration_pending(db_path): + vacuum_database(db_path) + return new_id def update_tooltip( @@ -992,6 +1025,9 @@ def update_tooltip( (category_id, tag, summary, detail, tooltip_id), ) conn.commit() + conn.close() + if _page_size_migration_pending(db_path): + vacuum_database(db_path) def get_button_number_ids(db_path: Path) -> list[int]: @@ -1020,6 +1056,9 @@ def add_tooltip_button( (tooltip_id, button_number_id, description, uri), ) conn.commit() + conn.close() + if _page_size_migration_pending(db_path): + vacuum_database(db_path) def update_tooltip_button( @@ -1042,6 +1081,9 @@ def update_tooltip_button( (description, uri, tooltip_id, button_number_id), ) conn.commit() + conn.close() + if _page_size_migration_pending(db_path): + vacuum_database(db_path) def delete_tooltip_button( diff --git a/docdb-studio/tests/test_vacuum.py b/docdb-studio/tests/test_vacuum.py index 8e5da1f6..4237ff6e 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -23,6 +23,10 @@ import_content_files = docdb_studio.import_content_files prescan_content_import = docdb_studio.prescan_content_import get_content_types = docdb_studio.get_content_types +insert_tooltip = docdb_studio.insert_tooltip +update_tooltip = docdb_studio.update_tooltip +add_tooltip_button = docdb_studio.add_tooltip_button +update_tooltip_button = docdb_studio.update_tooltip_button ImportItem = docdb_studio.ImportItem @@ -693,3 +697,119 @@ def cb(phase: str, current: int, total: int) -> None: assert "vacuum" in seen_phases finally: db.unlink(missing_ok=True) + + +# ---------- single-row tooltip mutations also migrate page_size ---------- + + +def test_insert_tooltip_migrates_page_size() -> None: + db = _make_tooltip_db(starting_page_size=1024) + try: + insert_tooltip(db, category_id=1, tag="new", summary="s", detail="d") + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + +def test_update_tooltip_migrates_page_size() -> None: + db = _make_tooltip_db(starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + conn.execute( + "INSERT INTO Tooltips (id, categoryId, tag, summary, detail) VALUES (1, 1, 'x', 's', 'd')" + ) + conn.commit() + update_tooltip(db, tooltip_id=1, category_id=1, tag="x", summary="s2", detail="d2") + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + +def test_add_tooltip_button_migrates_page_size() -> None: + db = _make_tooltip_db(starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + conn.execute( + "INSERT INTO Tooltips (id, categoryId, tag, summary, detail) VALUES (1, 1, 'x', 's', 'd')" + ) + conn.commit() + add_tooltip_button(db, tooltip_id=1, button_number_id=1, description="d", uri="u.html") + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + +def test_update_tooltip_button_migrates_page_size() -> None: + db = _make_tooltip_db(starting_page_size=1024) + try: + with sqlite3.connect(db) as conn: + conn.execute( + "INSERT INTO Tooltips (id, categoryId, tag, summary, detail) VALUES (1, 1, 'x', 's', 'd')" + ) + conn.execute( + "INSERT INTO TooltipButtons (tooltipId, buttonNumberId, description, uri) VALUES (1, 1, 'd', 'u.html')" + ) + conn.commit() + update_tooltip_button(db, tooltip_id=1, button_number_id=1, description="d2", uri="u2.html") + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + +# ---------- other vacuum_database robustness ---------- + + +def test_vacuum_database_handles_quote_in_tmp_dir_name(tmp_path: Path) -> None: + """VACUUM INTO's target is a SQL string literal, not a bindable parameter, + so an unescaped single quote anywhere in db_path's parent directory would + otherwise break the statement (e.g. a real user directory like "David's + Docs").""" + quote_dir = tmp_path / "David's Docs" + quote_dir.mkdir() + db = quote_dir / "test.db" + with sqlite3.connect(db) as conn: + conn.executescript( + "CREATE TABLE t (id INTEGER PRIMARY KEY, v TEXT); INSERT INTO t (v) VALUES ('x');" + ) + conn.commit() + vacuum_database(db) # must not raise + with sqlite3.connect(db) as conn: + (count,) = conn.execute("SELECT count(*) FROM t").fetchone() + assert count == 1 + + +def test_vacuum_database_concurrent_calls_do_not_raise() -> None: + """_vacuum_lock serializes vacuum_database against itself, so two threads + racing to vacuum the same db_path must not raise or corrupt the file.""" + import threading as _threading + + db = _make_tooltip_db(seed_filler_rows=20) + try: + errors: list[BaseException] = [] + + def run(): + try: + vacuum_database(db) + except BaseException as e: + errors.append(e) + + threads = [_threading.Thread(target=run) for _ in range(4)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert errors == [] + with sqlite3.connect(db) as conn: + (result,) = conn.execute("PRAGMA integrity_check").fetchone() + assert result == "ok" + finally: + db.unlink(missing_ok=True) From cac5cb802e534a5974137a4a0a7180c120a10db2 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 16:56:05 -0700 Subject: [PATCH 7/8] ADFA-5141: Use a bound parameter for VACUUM INTO's target Cleaner than manually escaping embedded single quotes: VACUUM INTO's target accepts a bound parameter (already used by populate_db.py's backup_database for the same reason), so this sidesteps SQL string-literal escaping entirely rather than hand-rolling it. Co-Authored-By: Claude Sonnet 5 --- docdb-studio/docdb_studio.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index 844ec471..62f27b71 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -743,15 +743,14 @@ def vacuum_database(db_path: Path) -> None: fd, tmp_name = tempfile.mkstemp(dir=db_path.parent, suffix=".vacuum.tmp") os.close(fd) tmp_path = Path(tmp_name) - # SQLite string-literal escaping (double any embedded single quote) -- - # VACUUM INTO takes its target as a string literal, not a bindable - # parameter, so a path containing a quote (e.g. a user's home directory - # named "David's Docs") would otherwise break the statement. - escaped_tmp_path = str(tmp_path).replace("'", "''") try: with sqlite3.connect(db_path, timeout=30.0) as conn: conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") - conn.execute(f"VACUUM INTO '{escaped_tmp_path}'") + # Bound parameter, not an f-string: VACUUM INTO's target + # accepts one, which sidesteps having to escape a path that + # contains a single quote (e.g. a user directory named + # "David's Docs"). + conn.execute("VACUUM INTO ?", (str(tmp_path),)) os.replace(tmp_path, db_path) os.chmod(db_path, original_mode) finally: From 73bd2af8b6a972b2d44f05b95d279eeb7bc585c3 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 17 Aug 2026 17:40:38 -0700 Subject: [PATCH 8/8] ADFA-5141: Fix findings from a third self-review - vacuum_database: chmod the temp file to the original permissions before os.replace, not after -- fixing it up afterward left a real (if brief) window where db_path was visible at mkstemp's 0600, and left permissions permanently wrong if the chmod itself failed. - vacuum_database: explicitly close the VACUUM INTO and WAL-reapply connections, and give the WAL-reapply connection the same 30s timeout as its siblings in the same function -- empirically reproduced a ResourceWarning: unclosed database for the two that were missed, and the untimed connection could abort the function after the rewrite/permissions/sidecar-cleanup already succeeded, leaving _page_size_confirmed never updated. - delete_tooltips_bulk: the empty-tooltip_ids early return skipped the migration-pending check entirely, unlike its non-empty "nothing matched" case which was already covered. - get_category_name/update_last_change: same 30s timeout as fetch_content_for_path, for the same reason -- they run right after replace_tooltip_buttons's unconditional vacuum in the tooltip-save flow. - Introduced _vacuum_if_dirty_or_pending() as a single choke point for the "vacuum after a mutation, or to migrate page_size on a pure insert" gate, replacing 6 near-identical copies of it (the 7th, import_content_files, keeps its own inline version since it wraps the gate with progress-callback reporting). A future new mutating helper can no longer add itself without also getting the migration check for free. New tests: the empty-list migration gap, plus verified (and reverted to confirm) that each fix here catches a real regression against the pre-fix code. Co-Authored-By: Claude Sonnet 5 --- docdb-studio/docdb_studio.py | 91 +++++++++++++++++++------------ docdb-studio/tests/test_vacuum.py | 15 +++++ 2 files changed, 72 insertions(+), 34 deletions(-) diff --git a/docdb-studio/docdb_studio.py b/docdb-studio/docdb_studio.py index 62f27b71..1dc120af 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -637,8 +637,14 @@ def get_categories(db_path: Path) -> list[tuple[int, str]]: def get_category_name(db_path: Path, category_id: int) -> str | None: - """Return category string for the given id, or None if not found.""" - with sqlite3.connect(db_path) as conn: + """Return category string for the given id, or None if not found. + + Uses the same 30s busy timeout as fetch_content_for_path (ADFA-5141): + called right after replace_tooltip_buttons's unconditional vacuum in the + tooltip-save flow, so a concurrent vacuum_database on the same db_path + (e.g. from another open window) can otherwise turn a transient lock into + a spurious failure on the default 5s timeout.""" + with sqlite3.connect(db_path, timeout=30.0) as conn: cur = conn.execute( "SELECT category FROM TooltipCategories WHERE id = ?", (category_id,), @@ -651,7 +657,11 @@ def update_last_change( db_path: Path, documentation_set: str, who: str | None ) -> None: """Stamp the LastChange row for the given documentationSet, plus the - global WHOLEDB_KEY row, in a single transaction.""" + global WHOLEDB_KEY row, in a single transaction. + + Uses the same 30s busy timeout as fetch_content_for_path (ADFA-5141) -- + see get_category_name's docstring, called right before this in the same + tooltip-save flow, for why.""" def _stamp(conn: sqlite3.Connection, doc_set: str) -> None: cur = conn.execute( @@ -670,7 +680,7 @@ def _stamp(conn: sqlite3.Connection, doc_set: str) -> None: (doc_set, who), ) - with sqlite3.connect(db_path) as conn: + with sqlite3.connect(db_path, timeout=30.0) as conn: _stamp(conn, documentation_set) if documentation_set != WHOLEDB_KEY: _stamp(conn, WHOLEDB_KEY) @@ -751,8 +761,12 @@ def vacuum_database(db_path: Path) -> None: # contains a single quote (e.g. a user directory named # "David's Docs"). conn.execute("VACUUM INTO ?", (str(tmp_path),)) + conn.close() + # chmod the temp file, not db_path, so the swap-in is atomic at + # the correct permissions -- fixing it up after os.replace would + # leave a window where db_path is visible at mkstemp's 0600. + os.chmod(tmp_path, original_mode) os.replace(tmp_path, db_path) - os.chmod(db_path, original_mode) finally: tmp_path.unlink(missing_ok=True) @@ -769,8 +783,9 @@ def vacuum_database(db_path: Path) -> None: # resulting sidecars are named correctly -- VACUUM INTO always # produces a plain rollback-journal file regardless of the # source's journal_mode. - with sqlite3.connect(db_path) as conn: + with sqlite3.connect(db_path, timeout=30.0) as conn: conn.execute("PRAGMA journal_mode=WAL") + conn.close() with _page_size_confirmed_lock: _page_size_confirmed.add(db_path) @@ -809,6 +824,18 @@ def _page_size_migration_pending(db_path: Path) -> bool: return pending +def _vacuum_if_dirty_or_pending(db_path: Path, dirty: bool) -> None: + """Shared choke point for the "vacuum after a mutation, or to migrate + page_size even on a pure-insert" gate (ADFA-5141), used by every mutating + call site except import_content_files (which wraps the same gate with its + own progress-callback reporting). One place means a future new mutating + helper can't add itself here without also getting the migration check -- + the exact gap this ticket found and fixed at each existing call site. + """ + if dirty or _page_size_migration_pending(db_path): + vacuum_database(db_path) + + def get_categories_for_tooltips( db_path: Path, tooltip_ids: list[int] ) -> set[str]: @@ -836,25 +863,26 @@ def get_categories_for_tooltips( def delete_tooltips_bulk(db_path: Path, tooltip_ids: list[int]) -> int: """Delete the given tooltip ids and their TooltipButtons rows. Returns rows deleted from Tooltips.""" - if not tooltip_ids: - return 0 deleted = 0 - with sqlite3.connect(db_path) as conn: - for batch in _chunked(tooltip_ids, SQL_PARAM_BATCH): - placeholders = ",".join("?" * len(batch)) - conn.execute( - f"DELETE FROM TooltipButtons WHERE tooltipId IN ({placeholders})", - batch, - ) - cur = conn.execute( - f"DELETE FROM Tooltips WHERE id IN ({placeholders})", - batch, - ) - deleted += cur.rowcount - conn.commit() - conn.close() - if deleted or _page_size_migration_pending(db_path): - vacuum_database(db_path) + if tooltip_ids: + with sqlite3.connect(db_path) as conn: + for batch in _chunked(tooltip_ids, SQL_PARAM_BATCH): + placeholders = ",".join("?" * len(batch)) + conn.execute( + f"DELETE FROM TooltipButtons WHERE tooltipId IN ({placeholders})", + batch, + ) + cur = conn.execute( + f"DELETE FROM Tooltips WHERE id IN ({placeholders})", + batch, + ) + deleted += cur.rowcount + conn.commit() + conn.close() + # Checked even for an empty tooltip_ids list (e.g. a "delete selected" + # action fired with nothing selected) -- this call site must not skip the + # one-time page_size migration just because there was nothing to delete. + _vacuum_if_dirty_or_pending(db_path, deleted) return deleted @@ -997,8 +1025,7 @@ def insert_tooltip( conn.commit() new_id = cur.lastrowid or 0 conn.close() - if _page_size_migration_pending(db_path): - vacuum_database(db_path) + _vacuum_if_dirty_or_pending(db_path, False) return new_id @@ -1025,8 +1052,7 @@ def update_tooltip( ) conn.commit() conn.close() - if _page_size_migration_pending(db_path): - vacuum_database(db_path) + _vacuum_if_dirty_or_pending(db_path, False) def get_button_number_ids(db_path: Path) -> list[int]: @@ -1056,8 +1082,7 @@ def add_tooltip_button( ) conn.commit() conn.close() - if _page_size_migration_pending(db_path): - vacuum_database(db_path) + _vacuum_if_dirty_or_pending(db_path, False) def update_tooltip_button( @@ -1081,8 +1106,7 @@ def update_tooltip_button( ) conn.commit() conn.close() - if _page_size_migration_pending(db_path): - vacuum_database(db_path) + _vacuum_if_dirty_or_pending(db_path, False) def delete_tooltip_button( @@ -1363,8 +1387,7 @@ def import_csv_rows( ) conn.commit() conn.close() - if updated or _page_size_migration_pending(db_path): - vacuum_database(db_path) + _vacuum_if_dirty_or_pending(db_path, updated) return inserted, updated diff --git a/docdb-studio/tests/test_vacuum.py b/docdb-studio/tests/test_vacuum.py index 4237ff6e..5cd7709c 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -304,6 +304,21 @@ def test_delete_tooltips_bulk_no_op_still_migrates_page_size() -> None: db.unlink(missing_ok=True) +def test_delete_tooltips_bulk_empty_list_still_migrates_page_size() -> None: + """An empty tooltip_ids list (e.g. a "delete selected" action fired with + nothing selected) used to early-return before the migration-pending check + ever ran -- must not skip the migration either.""" + db = _make_tooltip_db(seed_filler_rows=10, starting_page_size=1024) + try: + deleted = delete_tooltips_bulk(db, []) + assert deleted == 0 + with sqlite3.connect(db) as conn: + (page_size,) = conn.execute("PRAGMA page_size").fetchone() + assert page_size == docdb_studio.SQLITE_PAGE_SIZE_BYTES + finally: + db.unlink(missing_ok=True) + + def test_delete_tooltips_bulk_under_wal_does_not_deadlock() -> None: """SQLite refuses to switch a WAL-mode db away from WAL while any other connection (even one that already committed) is still open; delete_tooltips_bulk