ADFA-5153: Migration script for the shared Brotli dictionary - #1710
Closed
davidschachterADFA wants to merge 4 commits into
Closed
Conversation
…tation.db Recompresses every brotli Content row against the dictionary already in the database's CompressionDictionary table. Written for the 20-Aug database, which has the dictionary but plain-Brotli rows, so nothing benefits from it yet. Two things about the data decided the design, both checked rather than assumed: Content over 1 MiB is not stored as independently compressed pieces. The rows are raw 1 MiB slices of a single Brotli stream -- a slice alone does not decode -- so the unit of work is a base row plus its continuations, concatenated, decoded, recompressed and re-split. A naive per-row migration would have destroyed all three such items, silently, since each slice still looks like a blob. And those continuation rows are numbered from -2 while WebServer's reassembly loop starts at -1 (ADFA-5170), so they already serve truncated. The script preserves whatever numbering it finds, keeping the migration behaviour-neutral; --renumber-continuations rewrites from -1 instead, which makes them reachable again, as an opt-in rather than a side effect. Classification tries the plain decode first, deliberately: attaching no dictionary to a stream that needs one reliably fails, so a successful plain decode proves a row is unmigrated. The reverse is not safe -- a dictionary attached to a stream that never used one can decode to different bytes without erroring. A row that decodes identically both ways is left alone; those are tiny already-compressed payloads the compressor found nothing to reference for. Every item is verified before it is written: the recompressed bytes must decode back to exactly the original plaintext, or the item is reported as an error and left as it was. Measured on a copy of the 20-Aug database, 20 workers: 29,751 items, no errors, 129.0 MiB of stored content down to 85.7 MiB (33.6%), 3.3 minutes against about 73 single-threaded. The file itself goes 313.8 MB to 267.7 MB after VACUUM, and integrity_check passes. Verified independently of the script's own accounting: 303 sampled items, including all three chunked ones, decode with the dictionary to content byte-identical to what the source decodes plainly. Re-running is cheap (0.1 min) and converges -- pass two rewrote one row 11 bytes smaller, passes three and four changed nothing.
The `shell` block targets scripts/** wholesale and runs leadingSpacesToTabs(), so adding a .py file there gets it reindented to tabs -- against PEP 8, and against every .py already in this repo, all of which are space-indented. Only the ratchet has been hiding that: those files never differ from origin/stage, so Spotless never touches them. The first edit to scripts/cloudflare-r2-upload.py or scripts/insert-ci-perf-data.py would have silently converted the whole file, which is a trap worth removing rather than working around.
The dictionary migration now runs in three phases, because each changes what
the next one sees:
retype -- 74 rows hold GIF/PNG/JPEG/QuickTime payloads but are typed
text/plain (ADFA-5221), so they are Brotli-compressed for no gain
and served as Content-Type: text/plain. Store their plaintext and
point them at the type their magic bytes prove they are.
renumber -- 14 of 19 chunked items number continuations from -2 while the
app's reassembly loop starts at -1 (ADFA-5170), so they serve as
their first 1 MiB and nothing more. Shift them down.
migrate -- the existing recompression pass, unchanged.
Phase 1 feeds phase 3 for free: a row retyped to image/gif inherits that
type's compression = 'none', so the compression = 'brotli' selection stops
seeing it. No exclusion list needed.
Extensions only nominate phase 1's candidates; magic bytes decide, and a
name/content disagreement is reported rather than trusted. The four .mov files
are ftypqt QuickTime, not ISO-BMFF, so --mov-type chooses between the honest
video/quicktime (inserted into ContentTypes as id 28) and the video/mp4
Chromium is likelier to play.
Verified on a copy of the 20-Aug database: 74/74 retyped rows byte-identical
to the original plaintext, all 19 chunked items reassembling to unchanged
bytes, 250/250 sampled rows decoding with the dictionary to identical content,
integrity_check ok, no foreign-key violations, Content and Bookshelf row
counts unchanged, and a second run reporting nothing left to do. 3.5 min at 20
workers; 313.8 -> 268.1 MB after VACUUM.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
hal-eisen-adfa
marked this pull request as draft
August 21, 2026 18:44
ADFA-5171 is "Chunked Content rows numbered from -2 break reassembly"; ADFA-5170 is a separate task about peak heap when serving chunked rows. The docstring and the doc paragraph both pointed at the wrong one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Superseded by #1724. This PR was auto-closed when #1677 was squash-merged and its base branch |
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
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.
Stacked on #1677 — this PR's base is
feature/ADFA-5153-content-brotli-dictionary, so its diff is just the three commits below. GitHub retargets it tostageonce #1677 merges.A maintenance script that migrates an existing
documentation.dbonto the shared Brotli dictionary #1677 teachesWebServerto read. Nothing here ships in the APK; the only production file touched is a Spotless exclusion.Three phases, in this order
Each phase changes what the next one sees, so the order is load-bearing.
retypetext/plain(ADFA-5221), so they are Brotli-compressed for no gain and served asContent-Type: text/plain. Stores their plaintext and points them at the type their magic bytes prove.image/gif, 7 →image/png, 4 →video/quicktime, 2 →image/jpegrenumber-2while the reassembly loop starts at-1(ADFA-5171), so they serve as their first 1 MiB and nothing more. Shifts them down.-1migrateContentTypes.compression = 'brotli'row against the database's ownCompressionDictionary.Phase 1 feeds phase 3 for free: a row retyped to
image/gifinherits that type'scompression = 'none', so phase 3'scompression = 'brotli'selection stops seeing it. No exclusion list needed.Why it is safe to run incrementally
WebServertries a dictionary-attached decode first and falls back to a plain one, so a half-migrated database still serves every row. Classification deliberately tries the plain decode first: attaching no dictionary to a stream that needs one reliably fails, so a successful plain decode proves a row is not yet migrated. The reverse test is unsafe — a dictionary attached to a stream that never used one can decode to different bytes without erroring.Rows over 1 MiB are raw slices of one stream, not independently compressed pieces, so the unit of work is a logical item (base row plus continuations) concatenated, decoded, rewritten and re-split. Migrating such rows one at a time would destroy the content.
Verified on a copy of the 20-Aug database
.movreassembles from base +-1to the same 1,357,576 bytes; all 19 chunked items reassemble to unchanged bytes.contentTypeIDchanges and 14 renames, nothing else.Content30,649 → 30,649 rows,Bookshelf7 → 7 (the.pdfAddBook/DeleteBooktriggers never fire on continuation paths),PRAGMA integrity_checkok, no foreign-key violations.-1.VACUUM.Two judgement calls, both flags
--mov-type quicktime(default) inserts an honestvideo/quicktimeContentTypesrow. All four.movfiles are genuineftypqtQuickTime, which Chromium's demuxer generally will not play — so a correct type may still leave them blank.--mov-type mp4labels themvideo/mp4instead, which might coax playback. The real fix is transcoding indocdb-studio.--only-if-smallerstays off. Dictionary compression grows 9,098 rows by a median of 25 bytes — 257 KiB against 43.6 MiB saved — and turning it on would leave those rows plain and re-attempted on every future run.Both data defects originate in
docdb-studio's import path, so a freshly exported database carries them again until fixed there; ADFA-5221 and ADFA-5171 track that.Notes for review
build.gradle.ktsexcludes**/*.pyfrom the Spotlessshellblock, which was reindenting Python to tabs.docs/documentation-database.mdgains a paragraph on both data defects and which phase repairs each.🤖 Generated with Claude Code