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 7cce93c1..1dc120af 100644 --- a/docdb-studio/docdb_studio.py +++ b/docdb-studio/docdb_studio.py @@ -10,7 +10,9 @@ import os import platform as _platform import sqlite3 +import stat import sys +import tempfile import threading import time import unicodedata @@ -37,7 +39,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 @@ -633,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,), @@ -647,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( @@ -666,29 +680,161 @@ 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) conn.commit() + 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 by rewriting the DB file. Runs after every mutation - that includes a DELETE — DB hygiene is non-negotiable per project policy. - - 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. + + 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 + 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). 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 + 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. + + 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. """ - conn = sqlite3.connect(db_path, isolation_level=None) - try: - conn.execute("VACUUM") - finally: + 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: + (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) + try: + with sqlite3.connect(db_path, timeout=30.0) as conn: + conn.execute(f"PRAGMA page_size={SQLITE_PAGE_SIZE_BYTES}") + # 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),)) + 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) + 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, timeout=30.0) as conn: + conn.execute("PRAGMA journal_mode=WAL") + conn.close() + + 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: + """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. + """ + db_path = Path(db_path) + 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: + with _page_size_confirmed_lock: + _page_size_confirmed.add(db_path) + 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] @@ -717,24 +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() - if deleted: - 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 @@ -758,7 +906,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 @@ -871,7 +1023,10 @@ def insert_tooltip( (category_id, tag, summary, detail), ) conn.commit() - return cur.lastrowid or 0 + new_id = cur.lastrowid or 0 + conn.close() + _vacuum_if_dirty_or_pending(db_path, False) + return new_id def update_tooltip( @@ -896,6 +1051,8 @@ def update_tooltip( (category_id, tag, summary, detail, tooltip_id), ) conn.commit() + conn.close() + _vacuum_if_dirty_or_pending(db_path, False) def get_button_number_ids(db_path: Path) -> list[int]: @@ -924,6 +1081,8 @@ def add_tooltip_button( (tooltip_id, button_number_id, description, uri), ) conn.commit() + conn.close() + _vacuum_if_dirty_or_pending(db_path, False) def update_tooltip_button( @@ -946,6 +1105,8 @@ def update_tooltip_button( (description, uri, tooltip_id, button_number_id), ) conn.commit() + conn.close() + _vacuum_if_dirty_or_pending(db_path, False) def delete_tooltip_button( @@ -961,6 +1122,7 @@ def delete_tooltip_button( (tooltip_id, button_number_id), ) conn.commit() + conn.close() vacuum_database(db_path) @@ -993,6 +1155,7 @@ def replace_tooltip_buttons( (tooltip_id, button_number_id, description, uri), ) conn.commit() + conn.close() vacuum_database(db_path) @@ -1223,8 +1386,8 @@ def import_csv_rows( (tooltip_id, bid, label, uri), ) conn.commit() - if updated: - vacuum_database(db_path) + conn.close() + _vacuum_if_dirty_or_pending(db_path, updated) return inserted, updated @@ -1292,7 +1455,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]]: @@ -1420,6 +1585,7 @@ def prescan_content_import( else: orphan_set.add(base) orphan_row_ids.append(row_id) + conn.close() return ScanResult( mapped=list(candidates), @@ -1538,10 +1704,11 @@ 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) - 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) @@ -1645,7 +1812,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 +3719,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 +3743,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 +3774,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 +3830,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 +3855,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 +3900,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 +3984,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_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 4aebeaa4..5cd7709c 100644 --- a/docdb-studio/tests/test_vacuum.py +++ b/docdb-studio/tests/test_vacuum.py @@ -4,9 +4,14 @@ 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 + +import pytest import docdb_studio @@ -18,15 +23,26 @@ 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 -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 ( @@ -93,6 +109,154 @@ 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: + """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) + + +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 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.""" + + def __init__(self, real): + self._real = real + + def execute(self, sql, *args, **kwargs): + if sql.strip().upper().startswith("VACUUM INTO"): + raise sqlite3.OperationalError("simulated vacuum-into 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_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: _FailOnVacuumIntoConn( + real_connect(*a, **k) + ) + 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) + + def test_vacuum_database_shrinks_after_large_delete() -> None: db = _make_tooltip_db(seed_filler_rows=200) try: @@ -125,6 +289,54 @@ 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_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 + 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 ---------- @@ -149,6 +361,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 ---------- @@ -175,6 +411,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) ---------- @@ -201,14 +457,77 @@ 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) + + +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 + 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) ---------- -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); @@ -277,8 +596,81 @@ 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_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: - """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. @@ -320,3 +712,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) 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..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): @@ -27,6 +29,37 @@ 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_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: