ADFA-5153: Shared-dictionary Brotli compression for Content rows - #30
Merged
Merged
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. (cherry picked from commit 26c6250)
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. (cherry picked from commit 09ca170)
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. (cherry picked from commit 97755b1)
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. (cherry picked from commit 2827bfb)
Every finding from the three reviews on PR #26 that lands in files this PR touches, plus the one dictionary-consistency problem outside it that this PR itself creates. Data loss, both silent: * The migration deleted and re-inserted each base row. Content carries AddBook/DeleteBook triggers on '%.pdf' paths, so that cycle replaced every curated Bookshelf entry with 'CURRENT_TIMESTAMP || id' under a fresh Content.id -- verified on the real database: (53507, category 5, "Android Notes for Professionals") became (53508, category NULL, "2026-08-21 22:37:5553508"). 15 brotli-typed .pdf rows and all 7 Bookshelf rows are in scope. Writes are now UPDATE in place, with continuation rows reconciled by exact path. * delete_content interpolated a path straight into LIKE, where `_` is a wildcard and the `-%` suffix was not restricted to digits, so unrelated rows could be deleted permanently (hal-eisen-adfa). No write path goes through LIKE any more. Rows silently skipped while the run reported success: * reassemble_content probed "<path>-1", so an ADFA-5171 chain numbered from -2 reassembled truncated, failed to decode, and was counted as "already dictionary-compressed". The corpus has 29,751 base rows and exactly 3 with continuations; the run reported 29,748 migrated and 3 already-migrated in a first-ever migration, which is precisely those 3. Chain discovery is now shared with the repair script (populate_db.fragment_chain), so the two cannot drift apart again. * Any decode failure counted as "already migrated" (hal-eisen-adfa). Rows are now classified by decoding both ways: identical either way means the encoder never referenced the dictionary and there is nothing to gain (~0.5% of the real corpus, and the reason a second run used to re-migrate them -- alexmmiller); plain-only means migrate; dictionary-only means done; neither is an error, never a success. * Recompressed bytes are verified to round-trip before being written. Concurrency and memory: * Each worker opened its own read connection while the caller held one write transaction over the whole run, which deadlocks under journal_mode=delete -- documentation.db's actual mode (alexmmiller). All database access is now on the calling thread; workers receive bytes. Commits are batched, so an interrupted run keeps finished batches and resumes. * Blobs are no longer selected for every row up front (~130 MB held at once). Dictionary training, measured on the real corpus with only the sampling varied: first 300 rows by path (all under "a/") 36.2% smaller than plain 300 rows stratified across doc sets 33.2% <- worse stratified, 32 MiB plaintext budget 48.3% <- best first-by-path, same 32 MiB budget 36.4% <- volume alone: nil The docstring promised "a random sample drawn across the WHOLE Content table" and delivered the first 300 paths alphabetically -- 299 of them under "a/", while j/ (10,326 rows) and k/ (3,757) trained nothing (hal-eisen-adfa). Fixing it by stratifying alone makes things worse: quotas drawn from smaller doc sets starve the trainer, which then cannot even fill a 256 KiB dictionary. Both halves are needed, so sampling is now stratified by stored bytes and bounded by a plaintext budget, seeded for reproducibility since a stored dictionary is never retrained. renumber_misnumbered_fragments: * A chain numbered from -0 passed the "starts at 1?" guard and renamed onto an occupied slot, tripping UNIQUE(path) and rolling back every other repair in the pass (hal-eisen-adfa). Such a chain is repaired rather than skipped -- the app probes "-1", finds it, and serves the chain with "-0" dropped -- via a parking pass that is correct in either shift direction. docdb-studio: * sqlite3.OperationalError covers "database is locked", and caching that as "no dictionary" downgraded the whole session to plain Brotli (hal-eisen-adfa). Only definitive answers are cached now. * The new `brotli` CLI dependency raised RuntimeError/OSError out of paths that guard only `brotli.error` (hal-eisen-adfa). Missing-binary now raises a BrotliCliMissing subclass of brotli.error, with an actionable message. * decompress_brotli decoded dictionary-only, so it could not read plain rows -- which a dictionary database always contains: anything a plugin contributes on-device, anything written outside populate_db.py, and everything mid- migration. It now falls back to a plain decode, as WebServer.kt does. sync_kdoc_json_to_db (outside this PR's diff, but this PR is what makes documentation.db a dictionary database): * compress_for used plain brotli.compress, leaving every k/kotlin-stdlib row plain inside a dictionary database (hal-eisen-adfa). It now compresses against the database's dictionary when there is one. * "Source file missing => delete the row" had no floor: a Dokka layout change makes every lookup miss, and the script would delete every stdlib row plus its parent Tooltips and exit 0 (hal-eisen-adfa). Sources are resolved up front and a wholesale miss aborts. Corrected in populate_db's DictionaryCompressor docstring, because two reviews reasoned from it: the two mismatch directions are not alike. Decoding a dictionary row with NO dictionary is loud (398 of 400 real rows raised, 2 returned identical bytes, none wrong), which is what makes both this script's idempotency check and WebServer.kt's fallback sound. Decoding with the WRONG dictionary is the silent case (50% raised, 38% returned different bytes with no error, 12% identical). The test asserting a wrong-dictionary decode does not raise was asserting that coin flip; it now asserts the invariant that holds. Tests: 25 in ProcessKotlinWebsiteJSON (up from 21) and 173 in docdb-studio (up from 170) pass. New coverage for the -2 chain, an undecodable row, Bookshelf survival through the triggers, a never-referenced-dictionary row across two runs, stratified sample determinism and spread, zero-based renumbering, one bad chain not blocking other repairs, a locked database not being cached, a plain row in a dictionary database, and a missing brotli CLI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 838ac44)
… brotli Both from hal-eisen-adfa's follow-up review of 838ac44. insert_optimized_media.delete_content still built the LIKE pattern the migration script had stopped using: `path = ? OR path LIKE '<path>-%'`, where `_` is a single-character wildcard and the suffix is not constrained to digits. Rows matched that way are never re-inserted, so the loss is permanent. It now deletes the base row by exact path and each continuation by the exact paths populate_db.fragment_chain returns, which does the over-matching query once and re-checks every candidate's parsed suffix. The claim "no write path constructs a LIKE pattern any more" is now true of the whole tree, not just one file. The `brotli` CLI became a required external binary in three independent paths (populate_db's DictionaryCompressor, sync_kdoc_json_to_db, docdb_studio) and nothing declared it. The Python `brotli` package the README asks for is a different artifact and exposes no custom-dictionary parameter, which is exactly why the CLI is unavoidable -- and what makes `pip install brotli` read as though it covers this. Declared in the four places that would tell someone: * build-kotlin-docs.yaml's apt-get line -- it runs populate_db, insert_optimized_media and sync_kdoc_json_to_db. * docdb-regression-test.yaml's apt-get line -- it runs docdb-studio against the downloaded production database, which is now a dictionary database, so its reads need the binary too. (CI previously depended on whatever the runner image happened to ship.) * ProcessKotlinWebsiteJSON/README.md, beside the existing `pngquant on PATH` bullet, spelling out that this is the CLI and not the Python package. * docdb-studio/README.md, noting `uv sync` cannot install it and that a database with no CompressionDictionary needs nothing extra. publish-doc-db.yaml is deliberately untouched: it runs the scripts/ingest.py pipeline, which does not reach for the CLI. Tests unchanged and passing: 25 in ProcessKotlinWebsiteJSON, 173 in docdb-studio. Both workflow files still parse as YAML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 4d4f37d)
…base remint_dictionary.py trains a new shared dictionary for an already-migrated database and recompresses every 'brotli' row against it in one transaction, replacing the CompressionDictionary row. verify_remint_dictionary.py is the read-only gate: it decodes every row out of both databases and requires the plaintexts to match, exiting non-zero otherwise. These deliberately do what load_or_create_dictionary refuses to do, and the refusal is right for the pipeline: replacing a stored dictionary without recompressing the content orphans every row, since the dictionary decode fails and the plain fallback fails too. The only safe way to change a dictionary is to change the content with it, atomically, which is what this pair is for. Either every row converts and the dictionary is replaced, or nothing is written. Why it is worth having: the dictionary a database is first minted with is permanent for its content, so a poorly-sampled one stays expensive forever. Re-minting the 21-Aug database with the stratified, byte-budgeted sampler took its brotli content from 83.4 MiB to 65.6 MiB and the vacuumed file from 268 MB to 249 MB -- 18 MB -- with all 29,677 items verified byte-identical, and the result confirmed on device: pages served at their original byte counts through brotli4j, whose attachDictionary had never seen this dictionary before. The verifier is not ceremony. A row recompressed against a mismatched dictionary decodes with no error into *different* bytes 38% of the time (50% raises, 12% is identical because the perturbed region was never referenced), so nothing at runtime detects it and the check has to happen against the original before the file is put in place. collect_training_samples now takes an optional decoder, defaulting to plain Brotli. A re-mint's rows are dictionary-compressed, so it passes one that reads against the outgoing dictionary and falls back to plain -- the fallback is required, not defensive, because a dictionary database always holds some plain rows. read_item, write_item and load_base_rows are reused from the migration script rather than copied, which is what keeps the in-place write (never DELETE+INSERT on a base row, because of the '%.pdf' triggers) in one place. Four tests. Two of them exist because writing them corrected me: re-minting with the same seed and corpus reproduces the stored dictionary byte for byte, so a test asserting the dictionary changed has to vary the seed -- and an earlier assertion that the outgoing dictionary can no longer decode a re-minted row was asserting a coin flip, the same mistake as asserting that a wrong-dictionary decode raises. The remaining two cover the abort path leaving the database untouched, and the verifier actually objecting to a corrupted re-mint rather than passing vacuously. 29 pipeline tests (from 25) and 173 docdb-studio tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 4b19f14)
docdb-studio's README named `apt` and `brew` and left Windows users with nothing, on the one dependency `uv sync` cannot install for them. It now has a section of its own, following the per-OS shape the uv instructions already use: winget, scoop and choco, each preceded by the matching `search` command so a renamed package cannot strand the reader, plus MSYS2 for anyone who already has Git for Windows. Then the two things that actually go wrong on Windows: a changed PATH is only visible in newly-opened terminals, and a package manager can install the binary somewhere that is not on PATH at all -- so `where.exe brotli`, the usual shim directories, and where to edit PATH. It also states plainly that the `brotli` in `uv sync` is a different artifact from the `brotli` program, since `pip install brotli` succeeding is exactly what makes this confusing, and doubly so on Windows where there is no `brotli.exe` afterwards. Writing that section exposed a real defect in the BrotliCliMissing handling from 838ac44. Subclassing brotli.error kept a missing binary from escaping as an unhandled RuntimeError, which is what the review asked for -- but the two call sites catch brotli.error and return []/None, so the failure became a blank preview with nothing said anywhere. A corrupt row and a missing binary are not the same event: one is a single bad row, the other means nothing in this database will ever decode and is fixable in one command. The call sites now catch BrotliCliMissing separately and print which path failed and why, and the exception's message points at the README rather than listing two Unix package managers. The README says what actually happens -- blank preview plus an explanatory error in the launching terminal -- rather than claiming the UI reports it. 174 docdb-studio tests pass (from 173); the new one asserts both call sites log rather than swallow, and that the message names the path and points at the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 25d284f)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Splits the ADFA-5153 shared-dictionary Brotli work out of
fix/ADFA-4737, where ithad landed by accident, onto its own branch.
What this is
Eight commits cherry-picked from
fix/ADFA-4737, in their original order. Each onecarries a
(cherry picked from commit ...)trailer pointing at its original SHA:26c625009ca17097755b12827bfb838ac444d4f37d4b19f1425d284fThe six interleaved ADFA-5141 commits were deliberately excluded and are not needed here.
fix/ADFA-4737has not been modified. These commits still exist there too, sountil that branch is rebased the work is present in both places.
Read this before reviewing
This branch does not import or run standalone yet. It is based on
main, whichdoes not contain the
ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/tree —that arrives via PR #21. Consequences:
Files that these commits modify rather than create had to be imported whole,
so roughly 1,800 lines of ADFA-4737 pipeline code show up as new in this diff:
insert_optimized_media.py,populate_db.py, the pipelineREADME.md,sync_kdoc_json_to_db.py, andbuild-kotlin-docs.yaml. That is not new work andshould not be reviewed as such. It disappears from the diff once PR ADFA-4739: Pipeline for producing template-based Kotlin documentation #21 merges.
populate_db.pyimportsbuild_nav,md_to_json, andoptimize_media, whichlive only on the 4737 line. They are absent here, so
test_migrate_content_to_dictionary_brotli.py,test_populate_db_dictionary.py,and
test_remint_dictionary.pyfail at collection. They pass on the 4737 line.838ac44also touchedrenumber_misnumbered_fragments.py, which comes from358276d(ADFA-5171) — not part of this ticket. Those hunks were dropped and thefile is not present here.
Do not merge before PR #21. Merging this first would land a partial copy of the
Kotlin pipeline on
main.What was verified
fix/ADFA-4737(
25d284f) and match exactly, exceptdocdb_studio.py, which correctlythree-way-merged onto main's newer version of that file.
docdb-studio: 61 tests pass (test_compression_dictionary.py,test_content_import.py).