ADFA-5153: Compress Content table with a trained Brotli dictionary - #26
ADFA-5153: Compress Content table with a trained Brotli dictionary#26davidschachterADFA wants to merge 12 commits into
Conversation
populate_db.py trains a zstd fast-cover dictionary (256 KiB) from this run's own pages/nav on first use and stores it in a new CompressionDictionary table, then compresses every page/nav/image/asset row against it via the brotli CLI's -D flag (the installed Python brotli package has no dictionary API). Never retrains an existing dictionary: a dictionary-compressed row is only decodable against the exact dictionary it was compressed with, verified empirically to fail silently-wrong rather than loudly on a mismatch, so retraining would orphan every already-migrated row. insert_optimized_media.py rewrites the same rows populate_db.py writes (image optimization, in-place URL rewrites), so it now loads and reuses the same dictionary instead of the old plain-Brotli calls it would otherwise silently corrupt those rows with. ADFA-5153.
populate_db.py and insert_optimized_media.py only ever touch their own subset of Content (k/html/%, assets/%). Every other Content row -- reference docs, tooltip-linked pages, whatever else -- was still plain Brotli, no dictionary. migrate_content_to_dictionary_brotli.py recompresses every remaining 'brotli' row against the shared CompressionDictionary (training one from a representative whole-corpus sample if none exists yet), so the "every brotli row uses the dictionary" assumption WebServer.kt's reader depends on actually holds. Idempotent by construction: a plain decode reliably fails once a row is already dictionary-compressed (verified over 200 trials), so re-running is always a safe no-op. Backs up first (VACUUM INTO), runs in one transaction. Run against the real documentation.db: 29,748/29,751 brotli rows migrated, 131.1MB -> 85.6MB compressed, 299.0MB -> 255.3MB overall. ADFA-5153.
Every 'brotli' Content row in the real database is now compressed against the shared CompressionDictionary (see the prior two commits), but docdb_studio.py still read and wrote plain Brotli in three places: get_html_anchors_for_path, fetch_content_for_path (both decode), and compress_for_storage via import_content_files (encode). Against the migrated database this wasn't a latent risk -- it was already broken: a plain decode of dictionary-compressed content reliably fails, so anchor validation and content preview were silently erroring on every real page, and any new import would have written dictionary-incompatible plain Brotli back into a database that assumes there is none left. get_compression_dictionary(db_path) reads and caches a database's CompressionDictionary (or None, for a database that predates ADFA-5153) -- docdb-studio never creates or retrains one itself, only ever reads whatever another tool already produced. compress_for_storage/decompress_brotli shell out to the brotli CLI's -D flag when a dictionary is present, matching populate_db.py's approach, and fall back to the plain brotli package otherwise. decompress_brotli deliberately raises brotli.error on failure so the two existing call sites' `except brotli.error:` handling didn't need to change. Verified against the real (migrated) documentation.db: anchor lookup and content fetch both now work on real pages that previously would have errored. ADFA-5153.
Each row's recompress spawns its own `brotli` subprocess, so the ~30,000-row real migration was dominated by process-spawn overhead running strictly sequentially. Retrospective feedback: this should have been parallelized from the start rather than accepting a slow serial run. migrate() now runs reassemble+plain-decompress+dictionary-recompress on a ThreadPoolExecutor (defaults to ThreadPoolExecutor's own min(32, cpu_count+4), tuned for exactly this I/O/subprocess-bound shape); each worker opens its own read-only connection (a single sqlite3.Connection isn't safe across threads) and reuses one DictionaryCompressor per thread rather than one per row. The actual delete+insert writes stay serialized on the caller's connection, which SQLite requires anyway. Measured 3-6x faster than sequential on synthetic benchmarks. DictionaryCompressor gets an atexit safety-net close(), since a per-thread instance has no single call site that can cleanly scope a `with` block around it the way populate_db.py's/insert_optimized_media.py's own single-threaded usage already does. Test fixture switched from :memory: to a real temp file, since worker threads need an actual db_path to open their own connections against - an in-memory database has none and can't be shared across connections at all. ADFA-5153.
This is the pipeline that actually produces the live documentation.db (scripts/DocumentationDatabase.py, fixed earlier on this ticket, turned out to be dead code -- its tag-triggered workflow hasn't fired since db-2025-07-16b). populate_db.py has always run its own bare VACUUM with no page_size pin, so the real fix belongs here. Extracted vacuum_and_pin_page_size(), mirroring docdb_studio.py's vacuum_database(): pins page_size via PRAGMA before VACUUM, and works around WAL journal mode silently preventing PRAGMA page_size from taking effect (this file's own backup_database docstring already anticipates a live/WAL-mode database). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Piggybacking one more ADFA-5141 fix onto this PR (commit b203500), since it lands in the same file this PR already touches: While reviewing PR #25 (the docdb-studio side of ADFA-5141), I traced where the actually shipped The real pipeline is this branch's cc @alexmmiller since this builds on your |
vacuum_and_pin_page_size (commit b203500) mirrored docdb-studio.py's original vacuum_database(): in-place VACUUM + a journal_mode round-trip, which requires exclusive access to db_path. SQLite refuses to switch a WAL-mode database away from WAL while ANY other connection has it open -- even one from a function that has already returned, since Python's `with sqlite3.connect(...) as conn:` does not close conn on exit. Empirically reproduced and fixed the identical bug in docdb-studio.py's vacuum_database (PR #25); this mirrors that fix here since this pipeline's own VACUUM is the one actually run against the live documentation.db. Rewritten on VACUUM INTO: rebuild into a temp file next to db_path (read-only snapshot of the source, no exclusive access needed), then atomically swap it into place with os.replace. 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. Two new tests: the fix succeeds with both an unrelated open connection and an unclosed caller-style connection present at once (the actual scenario the old design was fragile against), and the original file is left untouched if VACUUM INTO fails partway (temp file cleaned up, no partial swap). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Found and fixed the identical bug in Rewritten on 17/17 local tests pass ( Note: this branch predates PR #25's |
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 of the mirrored docdb-studio.py fix caught this silently dropping documentation.db from 644 to 600 on every vacuum; same bug here since this pipeline's vacuum_and_pin_page_size uses the identical mkstemp+replace pattern. 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_and_pin_page_size (and fails against the pre-fix code, dropping to 600). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nal_mode-read connection Same fix as the mirrored docdb-studio.py version: VACUUM INTO's target accepts a bound parameter (already used by this file's own backup_database for the same reason), sidestepping SQL string-literal escaping for a path containing a single quote (e.g. "David's Docs") rather than hand-rolling it. Also explicitly closes the journal_mode -read connection instead of relying on it being reassigned by the next `with` block. New test: a quote in db_path's parent directory no longer breaks the statement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Follow-up: applied the same fixes here that a maintainer's QA and a follow-up self-review turned up on the mirrored PR #25 fix:
19/19 local tests pass. |
Same findings as the mirrored docdb-studio.py fix's third self-review: - chmod the temp file to the original permissions before os.replace, not after -- fixing it up afterward left a real window where db_path was visible at mkstemp's 0600, and left permissions permanently wrong if the chmod itself failed. - 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. 19/19 local tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Applied the applicable subset of PR #25's third self-review fixes here too: chmod-before-replace ordering and explicit connection closes with consistent timeouts. 19/19 local tests passing. |
…to ADFA-5153 Benchmarking showed page_size=1024 vs 2048 has essentially the same performance and a negligible size difference before compression (and likely less after this PR's dictionary compression) -- adding complexity without benefit. ADFA-5141 is declined; this PR is only about the Brotli dictionary compression (ADFA-5153) and the page_size work rode along on this branch by coincidence of timing, not by scope. Restores populate_db.py's original plain VACUUM call and removes vacuum_and_pin_page_size, SQLITE_PAGE_SIZE_BYTES, the now-unused os/stat imports, and their dedicated test file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
ADFA-5141 (the page_size=2048 change this PR's vacuum_and_pin_page_size was mirroring) has been declined — benchmarking showed essentially no performance difference between 1024 and 2048, and a negligible size difference before compression (likely even less after this PR's dictionary compression). Reverted that code out of this PR in |
WebServer.kt's reassembly loop always probes "<path>-1" first, but 14 of 19 chunked Content rows in the real documentation.db number their continuations starting at "-2" instead, with no "-1" row at all. The first lookup misses, the loop stops after the base 1 MB chunk, and the row is served short: a corrupt image (compression='none', silent 200) or a decode failure (compression='brotli', 500) - confirmed against a local copy of the shipped database (md5 34c879595bd6fb87e5b68989369680a8). No writer in this tool ever produced that numbering - populate_db.py, insert_optimized_media.py, and migrate_content_to_dictionary_brotli.py all go through insert_chunked_content, which has always started fragments at -1. This is inherited data older than this pipeline, not something it can regenerate correctly by re-running existing tools. renumber_misnumbered_fragments.py finds base rows whose fragment chain (via LIKE, sorted on the parsed numeric suffix rather than assumed paths) doesn't start at 1, and renumbers it to a contiguous run starting at -1, lowest-suffix first so each rename's target is the path just vacated by the previous one. A chain with an actual gap (a genuinely missing chunk, a different failure) is reported and left alone rather than guessed at. Content bytes are never touched, only paths, so it's safe regardless of a row's compression. Verified against a scratch copy of the real database: renumbers exactly the 14 chains the ticket found, and the two example rows (the devsite gif, the Javadoc index) reassemble and decode correctly afterward.
…bering ADFA-5171: Repair chunked Content rows misnumbered from -2
alexmmiller
left a comment
alexmmiller
left a comment
There was a problem hiding this comment.
QA'd this against the real ~300MB production documentation.db (post-ADFA-5141 page_size migration). The dictionary migration itself works and matches the PR's claimed numbers closely (29,748/29,751 rows, 131.08MB → 85.91MB, 34.5% smaller; spot-checked several real pages decode correctly through docdb-studio's fixed read paths). Found two bugs while doing so — left inline comments on each. Neither corrupted data (the first rolls back cleanly; the second just produces redundant, byte-identical work), but both are worth fixing before this ships.
|
|
||
| conn = sqlite3.connect(args.db_path) | ||
| try: | ||
| conn.execute("BEGIN") |
There was a problem hiding this comment.
Bug: this crashes with sqlite3.OperationalError: database is locked against a DB in rollback-journal mode (the default, and what our real production documentation.db actually uses — journal_mode=delete, not WAL).
migrate() runs the whole thing in one write transaction on conn, while each _migrate_one_row call (dispatched across a ThreadPoolExecutor, default up to min(32, cpu_count+4) workers) opens its own read connection to the same file (line ~165). Under journal_mode=WAL that's fine — readers and a writer don't block each other. But under rollback-journal mode, once this transaction's dirty-page cache overflows (very likely here: ~30k rows, ~85MB of new compressed content), SQLite has to escalate to an EXCLUSIVE lock to spill pages to the file, and that lock holds for the rest of the transaction. Every worker-thread read connection opened after that point fails with database is locked, executor.map raises, and the except Exception: conn.rollback() in main() discards all the work done so far — not silent corruption, but a guaranteed crash + total loss of progress on a real 300MB-class DB in the journal mode this project actually uses.
Reproduced this directly: it crashed partway through a real ~300MB copy of documentation.db (journal_mode=delete). Switching that copy to PRAGMA journal_mode=WAL first let the exact same script run to completion without any code changes. Worth either (a) documenting/enforcing that the target DB must be in WAL mode before running this, or (b) having the script itself switch to WAL for the duration and restore the original mode after, similar to how docdb_studio.vacuum_database already handles this WAL/rollback-journal distinction (ADFA-5141).
| worker_conn.close() | ||
|
|
||
| try: | ||
| plain = brotli.decompress(full) |
There was a problem hiding this comment.
Bug: idempotency doesn't hold for small/near-incompressible content, contrary to the module docstring's claim ("verified empirically over 200 trials").
The "already migrated" check is: try a plain (no-dictionary) brotli.decompress; success means "not yet migrated", failure means "already dictionary-compressed". That assumption breaks for small payloads where the encoder never actually needed to reference the custom dictionary — the resulting stream is then valid Brotli with or without it, so a plain decode of an already dictionary-compressed row can still succeed.
Concretely, on the real DB, re-running this script immediately after a fully successful migration (which should report migrated: 0) instead reported migrated: 228 (all .webp images plus k/version.html / k/version.json), with byte-identical before/after compressed sizes on every single one of those rows. Verified directly: brotli.decompress() on the already-migrated k/version.html blob succeeds and returns the correct plaintext, with no dictionary involved.
It's non-destructive (output is byte-for-byte identical, so nothing is lost), but every future re-run will burn CPU + spawn brotli subprocesses reprocessing this same ~228-row set forever, and the "idempotent, verified over 200 trials" claim in the docstring above should be scoped to note this known exception (or the detection logic should be hardened, e.g. checking for a minimum size before trusting the plain-decode heuristic, or storing a per-row/global migration marker instead of inferring it from decode behavior).
2 participants
Summary
Contentrows against it via thebrotliCLI's-Dflag (the installed Pythonbrotlipackage has no dictionary API). Stored in a new single-rowCompressionDictionarytable insidedocumentation.dbitself, so it always ships in sync with the content compressed against it.populate_db.py/insert_optimized_media.py: the Kotlin-website ingestion pipeline now compresses/decompresses through the shared dictionary, never retraining an existing one (a dictionary-compressed row is only decodable against the exact dictionary it was compressed with — verified empirically to fail, or silently produce different bytes, on any mismatch).migrate_content_to_dictionary_brotli.py: one-time, idempotent, whole-database migration for everyContentrow the above pipeline doesn't own (reference docs, tooltip-linked pages, etc.). Idempotent because a plain decode of already-migrated content reliably fails (verified over 200 trials).docdb-studio/docdb_studio.py: fixes a real correctness gap this surfaced — its Content reads (get_html_anchors_for_path,fetch_content_for_path) and writes (compress_for_storage) were still plain Brotli, which was already broken against the migrated database (anchor validation / content preview were silently erroring on every real page). Now dictionary-aware, same approach aspopulate_db.py, with a graceful fallback for a database with no dictionary yet.documentation.db: 29,748/29,751 brotli rows migrated, compressed bytes 131.1MB → 85.6MB (34.7% smaller), file overall 299.0MB → 255.3MB. Spot-checked real pages post-migration (both via the migration script and viadocdb-studio) — all decode correctly.Companion PR: appdevforall/CodeOnTheGo#1677 (WebServer.kt read-side).
Note:
scripts/DocumentationDatabase.pylooks obsolete against the current schema (its own schema-conformance check would reject this database outright — notemplateId/Templatesawareness) — flagged for a follow-up decision on deletion, not touched here.Test plan
docdb-studio:uv run pytest tests/— 170/170 pass (162 existing + 8 new)ProcessKotlinWebsiteJSON:python3 -m unittestacross both new test files — 21/21 passmigrate_content_to_dictionary_brotli.pyagainst the realdocumentation.db, verified idempotent (second run: 0 migrated, all already-dictionary-compressed)docdb-studio's fixed read paths