diff --git a/.github/scripts/patch_fan_lib.sh b/.github/scripts/patch_fan_lib.sh index b9e1d069..49985d8d 100644 --- a/.github/scripts/patch_fan_lib.sh +++ b/.github/scripts/patch_fan_lib.sh @@ -39,6 +39,16 @@ read_schema() { printf '%s' "$v" } +# Why an anchor shipped no patch has to outlive its own subshell: the driver's +# final "no patch at all" check must tell a size-guard skip (still a +# publishable full-only release) from a broken fan. +SKIP_DIR="$RUNNER_TEMP/patch-fan-skips" +rm -rf "$SKIP_DIR" +mkdir -p "$SKIP_DIR" +record_skip() { # + printf 'anchor v%s: %s\n' "$2" "$3" > "$SKIP_DIR/v$2.$1" +} + # One anchor, start to finish: pre-check, reconstitute, produce, # verify, clean up. Every path it touches is scoped to its own offset # so two of these can run side by side, it writes to its own log, and @@ -53,7 +63,7 @@ produce_anchor() { # local PREV_DB="$ANCHOR_DIR/seforim.db" local PATCH_OUT="$PWD/patches/patch-v${TARGET_VER}-v${THIS_VER}.db" local PRECHECK WAIT_BUDGET PREFETCH_STATE PREFETCH_WAITED - local PREV_SCHEMA PRODUCE_RC REASON + local PREV_SCHEMA PRODUCE_RC REASON SKIP_KIND local T_START T_DOWNLOADED T_EXTRACTED T_DONE echo "=== Producing patch v${TARGET_VER} → v${THIS_VER} (offset $OFFSET, tag=$TAG) ===" @@ -95,6 +105,7 @@ produce_anchor() { # echo "pre-check: $PRECHECK" if [ "${PRECHECK%% *}" = UNPATCHABLE ]; then echo "::warning::anchor v${TARGET_VER} ($TAG): ${PRECHECK#* } — pre-download schema check declared the anchor unpatchable; skip anchor" + record_skip structural "$TARGET_VER" "${PRECHECK#* }" return 0 fi @@ -175,6 +186,7 @@ produce_anchor() { # echo "schema $PREV_SCHEMA → $THIS_SCHEMA — producing the supported cross-schema delta" else echo "schema $PREV_SCHEMA → $THIS_SCHEMA is unsupported — skip anchor" + record_skip structural "$TARGET_VER" "schema $PREV_SCHEMA → $THIS_SCHEMA is unsupported" rm -rf "$ANCHOR_DIR" return 0 fi @@ -231,6 +243,11 @@ produce_anchor() { # if [ "$PRODUCE_RC" -eq 3 ] || [ -f "$PATCH_OUT.unpatchable" ]; then REASON=$(cat "$PATCH_OUT.unpatchable" 2>/dev/null || true) echo "::warning::anchor v${TARGET_VER} ($TAG): ${REASON:-see PatchPipelineCli output} — producer declared the anchor unpatchable; skip anchor" + # PatchSizeGuard.MARKER_REASON_TOKEN leads the marker when the + # delta was merely too big — a full-only release, not a defect. + SKIP_KIND=structural + case "$REASON" in oversized-delta:*) SKIP_KIND=oversized ;; esac + record_skip "$SKIP_KIND" "$TARGET_VER" "${REASON:-see PatchPipelineCli output}" # Leave nothing behind: the marker plus the producer's half-built # .tmp (and any stale .db) must not clutter patches/, which the # release staging step globs. diff --git a/.github/scripts/test_manual_release_workflow.py b/.github/scripts/test_manual_release_workflow.py index 073f9c33..b6516c19 100644 --- a/.github/scripts/test_manual_release_workflow.py +++ b/.github/scripts/test_manual_release_workflow.py @@ -48,6 +48,11 @@ / "io" / "github" / "kdroidfilter" / "seforimlibrary" / "common" / "patch" / "PatchPipelineCli.kt" ) +PATCH_SIZE_GUARD = ( + Path(__file__).parents[2] + / "generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary" + / "common/patch/PatchSizeGuard.kt" +) class ManualReleaseWorkflowContractTest(unittest.TestCase): @@ -278,6 +283,50 @@ def test_patch_fan_skips_only_anchors_the_producer_declares_unpatchable(self): patch_fan.index("patch fan produced no patch although prior releases exist"), ) + def test_a_corpus_wide_size_guard_skip_still_publishes_a_full_only_release(self): + # A build that churns every line legitimately loses every anchor to the + # delta size guard (Otzaria issue #1211). That is a degraded release — + # seforim.db.zst + buildstate, no patches — not a broken patch contract. + patch_fan = self.step("Produce + verify patch fan") + # produce_anchor lives in patch_fan_lib.sh, sourced into the step's shell. + fan_lib = self.fan_lib + + # The kind of every skip outlives its subshell as a file, because the + # anchors run in background subshells that cannot share a variable. + self.assertIn('SKIP_DIR="$RUNNER_TEMP/patch-fan-skips"', fan_lib) + self.assertIn("record_skip() {", fan_lib) + # Every skip path records, so "no patch and no recorded skip" stays an error. + self.assertEqual(fan_lib.count("record_skip "), 3) + self.assertIn('record_skip structural "$TARGET_VER" "${PRECHECK#* }"', fan_lib) + self.assertIn('record_skip "$SKIP_KIND" "$TARGET_VER"', fan_lib) + + # The oversized/structural split is driven by the token PatchSizeGuard + # puts at the head of its marker — the two must not drift apart. + token = re.search( + r'MARKER_REASON_TOKEN: String = "([^"]+)"', + PATCH_SIZE_GUARD.read_text(encoding="utf-8"), + ) + self.assertIsNotNone(token, "PatchSizeGuard must expose MARKER_REASON_TOKEN") + self.assertIn( + f'case "$REASON" in {token.group(1)}:*) SKIP_KIND=oversized ;; esac', + fan_lib, + ) + + # No patch + at least one size-guard skip: warn and carry on. + warn_at = patch_fan.index("::warning::patch fan produced no patch:") + err_at = patch_fan.index("patch fan produced no patch although prior releases exist") + self.assertLess(warn_at, err_at) + self.assertIn('if [ "$OVERSIZED_SKIPS" -gt 0 ]; then', patch_fan) + self.assertNotIn("exit 1", patch_fan[warn_at:err_at]) + # No patch and no size-guard skip is still a hard failure. + self.assertIn("exit 1", patch_fan[err_at:]) + + # Staging a release with zero patches is already guarded. + self.assertIn( + 'if compgen -G "patches/patch-*.db.zst" > /dev/null; then', + self.step("Stage release assets"), + ) + def test_patch_fan_decides_unpatchable_anchors_before_downloading_them(self): # An anchor the producer will reject costs 110–135 s of download plus a # decompress before anyone learns that (run 33865604251, anchor v10). diff --git a/.github/workflows/delta-pipeline-dryrun.yml b/.github/workflows/delta-pipeline-dryrun.yml index 93c19f37..1f4e5a8a 100644 --- a/.github/workflows/delta-pipeline-dryrun.yml +++ b/.github/workflows/delta-pipeline-dryrun.yml @@ -110,11 +110,14 @@ jobs: # catalog.pb is no longer produced; drop any stale copy so it can't # be auto-embedded into the patch via the default -PcatalogPb path. rm -f build/catalog.pb + # These runs test the delta MECHANICS on toy DBs, where a patch is + # legitimately huge relative to the DB — the size guard must not fire. ./gradlew :generator-common:producePatchAndVerify \ -PprevDb=$PWD/build/seforim.db.v1 \ -PnewDb=$PWD/build/seforim.db \ -Pout=$PWD/build/patch-v1-v2.db \ -PfromVersion=1 -PtoVersion=2 \ + -PmaxDeltaUncompressedRatio=1000 \ --no-daemon --stacktrace # ─── PATCH SIZE + MANIFEST SUMMARY ────────────────────────────────────── diff --git a/.github/workflows/delta-real-diff-arm.yml b/.github/workflows/delta-real-diff-arm.yml index 37790770..bccf7e15 100644 --- a/.github/workflows/delta-real-diff-arm.yml +++ b/.github/workflows/delta-real-diff-arm.yml @@ -223,11 +223,14 @@ jobs: # catalog.pb is no longer produced; drop any stale copy so it can't # be auto-embedded into the patch via the default -PcatalogPb path. rm -f build/catalog.pb + # These runs test the delta MECHANICS on toy DBs, where a patch is + # legitimately huge relative to the DB — the size guard must not fire. ./gradlew :generator-common:producePatchAndVerify \ -PprevDb=$PWD/build/seforim.db.v1 \ -PnewDb=$PWD/build/seforim.db \ -Pout=$PWD/build/patch-v1-v2.db \ -PfromVersion=1 -PtoVersion=2 \ + -PmaxDeltaUncompressedRatio=1000 \ --no-daemon --stacktrace - name: Summarise patch diff --git a/.github/workflows/delta-real-diff-test.yml b/.github/workflows/delta-real-diff-test.yml index ee303944..ed120cfa 100644 --- a/.github/workflows/delta-real-diff-test.yml +++ b/.github/workflows/delta-real-diff-test.yml @@ -223,11 +223,14 @@ jobs: # catalog.pb is no longer produced; drop any stale copy so it can't # be auto-embedded into the patch via the default -PcatalogPb path. rm -f build/catalog.pb + # These runs test the delta MECHANICS on toy DBs, where a patch is + # legitimately huge relative to the DB — the size guard must not fire. ./gradlew :generator-common:producePatchAndVerify \ -PprevDb=$PWD/build/seforim.db.v1 \ -PnewDb=$PWD/build/seforim.db \ -Pout=$PWD/build/patch-v1-v2.db \ -PfromVersion=1 -PtoVersion=2 \ + -PmaxDeltaUncompressedRatio=1000 \ --no-daemon --stacktrace - name: Summarise patch diff --git a/.github/workflows/manual-generate-release.yml b/.github/workflows/manual-generate-release.yml index 270e7c34..26b5daab 100644 --- a/.github/workflows/manual-generate-release.yml +++ b/.github/workflows/manual-generate-release.yml @@ -2061,12 +2061,21 @@ jobs: wait buildstate 3600 echo "=== Final patch artefacts ===" ls -lh patches/ || true - # Skipped anchors are warnings, but a release with prior versions and - # NO delta at all means every anchor was skipped — that is a broken - # patch contract, not a degraded one, and must fail loudly. + # Skipped anchors are warnings. No delta AT ALL is only acceptable + # when the size guard is what removed them: a corpus-wide churn build + # legitimately ships full-only (seforim.db.zst + buildstate). Every + # anchor skipped for any other reason is still a broken patch + # contract and must fail loudly. SKIP_DIR is patch_fan_lib.sh's. if ! compgen -G "patches/patch-*.db.zst" > /dev/null; then - echo "::error::patch fan produced no patch although prior releases exist (every anchor skipped)" - exit 1 + OVERSIZED_SKIPS=$(find "$SKIP_DIR" -type f -name '*.oversized' | wc -l) + STRUCTURAL_SKIPS=$(find "$SKIP_DIR" -type f -name '*.structural' | wc -l) + if [ "$OVERSIZED_SKIPS" -gt 0 ]; then + echo "::warning::patch fan produced no patch: $OVERSIZED_SKIPS anchor(s) dropped by the delta size guard, $STRUCTURAL_SKIPS structurally unpatchable — publishing a full-only release (seforim.db.zst + buildstate)" + cat "$SKIP_DIR"/*.oversized + else + echo "::error::patch fan produced no patch although prior releases exist (every anchor skipped; $STRUCTURAL_SKIPS structurally unpatchable, none dropped by the size guard)" + exit 1 + fi fi # Compress the full DB into a single .zst for the release. Done after the diff --git a/DELTA_UPDATE_WORKFLOW.md b/DELTA_UPDATE_WORKFLOW.md index 4464de60..2ca5e0f4 100644 --- a/DELTA_UPDATE_WORKFLOW.md +++ b/DELTA_UPDATE_WORKFLOW.md @@ -113,7 +113,7 @@ The allocator's natural keys are : | `tocText` | display text | | | `connection_type` | `name` | "commentary", "targum" | | `book` | `(sourceName, canonicalHeTitle)` | survives renames via book_aliases | -| `line` | `(bookId, "REF:"+heRef)` for Sefaria, `(bookId, contentHash, occurrenceIdx)` for Otzaria | heRef is THE killer feature — survives Sefaria's prefix renumbering | +| `line` | `(bookId, sha1("CT:"+rawSegment), occurrenceIdx)` | content only — see the note below | | `tocEntry` | `(bookId, ancestorPath@lineIndex)` | path is a `/`-joined sequence of tocText ids | | `link` | `(srcLineId, tgtLineId, connectionTypeId)` | | @@ -121,6 +121,30 @@ The allocator's natural keys are : > contents are unchanged, all ids match. If a row's content changes, > only that row gets a new id; everything around it stays put. +> **`line` key, post-#1211.** Both Sefaria and Otzaria lines are keyed on +> content alone. Up to db v27 a Sefaria line with an `heRef` was keyed +> `"REF:"+heRef` instead; one reformatting pass that rewrote every heRef +> renumbered the whole corpus and produced a 3 GB delta, so the ref — an +> ordinary updatable column — is out of the key. +> +> `rawSegment` is the Sefaria segment **before** the generated prefixes the +> importer injects (`(א) `, daf labels). Hashing the rendered line would +> reintroduce the same failure at chapter scale: inserting one verse +> reprefixes every verse after it. `BookPayload.cleanShiftByLineIndex` +> records the injected prefix length, and the key strips it. +> +> Lines that repeat verbatim inside one book are separated by +> `occurrenceIdx`, a per-`(bookId, contentHash)` counter in document order. +> +> `LegacyLineKey` is the one-build migration shim: on a miss the allocator +> retries the pre-#1211 key (`"REF:"+heRef`, else `"CT:"+renderedContent`) +> for **every** line and re-files the id under the new key, so the snapshot +> it writes is fully migrated. The shim reads only the *seed* snapshot, and an +> id already issued to another line this build is never handed out again +> (`legacyLineKeyCollisions` in the build summary) — a line whose seed id was +> taken gets a fresh one rather than a duplicate. Delete the shim once no +> build_state in circulation predates the change. + ### 1.2 The full producer pipeline ```mermaid diff --git a/LINKER_DELTA_PLAN.md b/LINKER_DELTA_PLAN.md index b0b44366..a98cb313 100644 --- a/LINKER_DELTA_PLAN.md +++ b/LINKER_DELTA_PLAN.md @@ -25,10 +25,13 @@ ל-שורה **בזמן בניית ה-DB** דרך ה-resolver הקיים של הגנרטור. כך: - כשספריא משנה ספר-יעד, ה-ref פשוט נפתר לשורה החדשה — **בלי לגעת בקישור**. -- הגנרטור כבר ממפתח `lineId` של שורות ספריא לפי **`REF:$heRef`** - (`IdAllocatorBindings.lineNaturalKeyHash`), כך ש-**כל עוד ה-heRef של הפסוק לא - השתנה, ה-`lineId` יציב בין builds → ה-`linkId` יציב → הקישור שורד עדכון-תוכן - של ספריא ללא שום churn ב-delta.** +- הגנרטור ממפתח `lineId` לפי **תוכן השורה הגולמי** בלבד + (`IdAllocatorBindings.lineNaturalKeyHash` = `sha1("CT:"+rawSegment)`, ללא + ה-heRef וללא הקידומת המיוצרת), כך ש-**כל עוד טקסט הפסוק לא השתנה, ה-`lineId` + יציב בין builds → ה-`linkId` יציב → הקישור שורד עדכון-מבנה או שינוי-ref של + ספריא ללא שום churn ב-delta.** + > עד db v27 המפתח היה `REF:$heRef`, ומעבר-עיצוב שכתב מחדש כל heRef מספרר + > מחדש את כל הקורפוס (issue #1211). ה-ref הוא עמודה מתעדכנת, לא מפתח. זו התשובה ל"התאמת קישורים לספרים שהשתנו": **ההתאמה אוטומטית דרך פתרון-מחדש**, לא הגירה. הצעד היקר (NER) מתבצע רק על ספרי-**מקור** שהשתנו. @@ -111,7 +114,7 @@ | המרה + מיזוג (מחליף רק ערכי "linker") | `otzaria-library/linker/to_otzaria_links.py` | ⚠️ קיים, **מושבת** | | פתרון ref→שורה | `SefariaImportRefs.resolveRefs` (+ `refsByCanonical/refsByBase`) | ✅ קיים, בדוק | | ‏id יציב + patch DB-ל-DB | `InMemoryIdAllocator.linkId`, `PatchDbProducer`, `PatchTables` | ✅ קיים | -| ‏line-id יציב לפי ref | `lineNaturalKeyHash` = `REF:$heRef` | ✅ קיים (הלב של היציבות) | +| ‏line-id יציב לפי תוכן | `lineNaturalKeyHash` = `sha1("CT:"+rawSegment)` | ✅ קיים (הלב של היציבות) | | מעקב ספרי-מקור שהשתנו בבנייה | `SourceHashComputer` + `TouchedBookDetector` + `BookRenameDetector` | ✅ קיים | **המסקנה:** אין לבנות תשתית delta מאפס — יש **להחיות ולחבר**. @@ -173,23 +176,23 @@ lineIndex`) **וגם את `lineId` המוטבע לכל רשומה** (זהות-ה | שינוי | השפעה | טיפול | עלות | |---|---|---|---| | **ספר-מקור (otzaria) שינה תוכן** | מיקומי ציטוט זזים | הרצת לינקר מחדש על הספר בלבד | דקות (NER על ספר) | -| **ספר-יעד (ספריא) שינה תוכן, heRef נשמר** | השורה זזה | פתרון-מחדש בבנייה; `lineId` יציב (REF-keyed) | **אפס** — קישור שורד, בלי churn | -| **ספר-יעד שינה מבנה (heRef השתנה/נמחק)** | ref לא נפתר | קישור מתעדכן/נופל + מדווח (כמו קישורי ספריא) | אפס לינקר | +| **ספר-יעד (ספריא) שינה מבנה/heRef, טקסט הפסוק נשמר** | השורה זזה | פתרון-מחדש בבנייה; `lineId` יציב (content-keyed) | **אפס** — קישור שורד, בלי churn | +| **ספר-יעד שינה את טקסט הפסוק עצמו** | `lineId` מתחלף | הקישור נמחק+נכתב מחדש ב-patch | churn מקומי (הפסוק אכן השתנה) | | **ספר-יעד שונה-שם/הועבר** | ref-מחרוזת מיושן | rewrite לפי `changelog_diff.json` | זול (מחרוזות) | | **ספר-מקור שונה-שם/נמחק** | קובץ-ארטיפקט מיושן | move/delete (לוגיקת `linker_on_commit.py`) | זול | -> **הערך:** התרחיש התכוף והמסוכן ביותר — עדכון-תוכן של ספר-יעד ספריא — +> **הערך:** התרחיש התכוף והמסוכן ביותר — עדכון-מבנה של ספר-יעד ספריא — > עולה **אפס** בלינקר ואינו שובר קישורים, בזכות שילוב ref-artifact + line-id -> ממופתח-heRef. זה מה שהופך את זה מ"שעות ריצה שמתפוצצות" ל"בנייה אינקרמנטלית". +> ממופתח-תוכן. זה מה שהופך את זה מ"שעות ריצה שמתפוצצות" ל"בנייה אינקרמנטלית". -### 2ה. מגבלה כנה — צד-המקור של ספרי otzaria +### 2ה. מגבלה כנה — עריכת טקסט מחליפה `lineId` -שורות של ספרי otzaria ממופתחות ל-`lineId` לפי **hash-תוכן** (`stableLineId` → -`normalisedContentHash`, בלי קידומת ref). לכן עריכה של שורת-מקור otzaria מחליפה -את `srcLineId` → `linkId` משתנה → הקישור נמחק+נכתב מחדש ב-patch (churn) גם אם -הציטוט טקסטואלית זהה. זה **מקובל** (הספר השתנה) אבל מייצר delta גדול מהמינימלי. -שיפור עתידי אפשרי: מפתח-טבעי יציב יותר לשורות otzaria. **לא חוסם** — היציבות -החשובה (צד-יעד/עדכוני-ספריא, המקרה התכוף) פתורה במלואה. +כל השורות, בשני הצדדים, ממופתחות ל-`lineId` לפי **hash-תוכן**. לכן עריכה של +שורת-מקור מחליפה את `srcLineId` → `linkId` משתנה → הקישור נמחק+נכתב מחדש +ב-patch (churn) גם אם הציטוט טקסטואלית זהה. זה **מקובל** (השורה אכן השתנתה) +אבל מייצר delta גדול מהמינימלי. שיפור עתידי אפשרי: התאמת-שורות מנורמלת +(`LineMatcher`) שתשמר id על עריכה קלה. **לא חוסם** — היציבות החשובה (שינויי +מבנה ו-ref בספריא, המקרה התכוף) פתורה במלואה. --- diff --git a/generator/common/build.gradle.kts b/generator/common/build.gradle.kts index ab645890..55a0a48f 100644 --- a/generator/common/build.gradle.kts +++ b/generator/common/build.gradle.kts @@ -103,7 +103,7 @@ tasks.register("producePatchAndVerify") { listOf( "releaseMeta", "fullBundleUrl", "fullBundleSha", "fullBundleSize", "manifestBaseUrl", "fromSchemaVersion", "toSchemaVersion", - "catalogPb", "zstdLevel", + "catalogPb", "zstdLevel", "maxDeltaUncompressedRatio", ).forEach { key -> project.findProperty(key)?.let { systemProperty(key, it as String) } } diff --git a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocator.kt b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocator.kt index c03a24dd..e27a7dd0 100644 --- a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocator.kt +++ b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocator.kt @@ -17,8 +17,6 @@ import java.nio.file.Path * Contract: for a given natural key, [bookId] / [lineId] / etc. MUST return the * same value across builds. New natural keys get a fresh id from a per-table * monotonic counter. Implementations are thread-safe. - * - * See DELTA_UPDATE_PLAN.md §3.3 and §3.5. */ interface IdAllocator { @@ -49,6 +47,13 @@ interface IdAllocator { // ─── Composite-keyed tables ──────────────────────────────────────────────── fun bookId(sourceName: String, canonicalHeTitle: String): Long fun lineId(bookId: Long, contentHash: ByteArray, occurrenceIdx: Int): Long + + /** + * As [lineId], but reuses the id stored under [legacy] when the new-scheme + * key is unknown. Transition shim — see [LegacyLineKey]. + */ + fun lineId(bookId: Long, contentHash: ByteArray, occurrenceIdx: Int, legacy: LegacyLineKey?): Long = + lineId(bookId, contentHash, occurrenceIdx) fun tocEntryId(bookId: Long, ancestorPath: String): Long fun altTocStructureId(bookId: Long, key: String): Long fun altTocEntryId(structureId: Long, ancestorPath: String): Long diff --git a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocatorBindings.kt b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocatorBindings.kt index 73594cdb..9150a673 100644 --- a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocatorBindings.kt +++ b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/IdAllocatorBindings.kt @@ -18,8 +18,6 @@ import java.util.concurrent.ConcurrentHashMap * the row once and returns the same id. This relies on repo `*WithId` methods * using `ON CONFLICT DO NOTHING` for lookup tables, and on the existing * `entity.id > 0` short-circuit for book/line/tocEntry/altToc. - * - * See DELTA_UPDATE_PLAN.md §3.5. */ class IdAllocatorBindings( val allocator: IdAllocator, @@ -204,21 +202,16 @@ class IdAllocatorBindings( /** * Builds the 20-byte content-hash slot of a line's natural key. * - * When a Sefaria-style stable citation reference is available (heRef - * like "Genesis 1:1"), we hash `"REF:$ref"` so the natural key is - * decoupled from the rendered content — Sefaria's pipeline auto- - * generates verse prefixes `(א), (ב), …` that mutate the rendered - * content of every following verse on a head-insert. By keying on - * heRef we keep the line id stable across those reformatting passes - * (see DELTA_UPDATE_PLAN.md §2.1 + PHASE1_VALIDATION.md Test C). + * Content only: heRef is a rendered label that a formatting pass can + * rewrite for a whole corpus at once, and keying on it renumbered every + * line of every simple-schema book in v27 (issue #1211). Lines that + * repeat inside a book are separated by the occurrence index instead. * - * Otzaria lines and Sefaria heading lines fall back to a raw content - * hash since they have no stable citation address. + * [content] must be the RAW segment: a generated prefix ("(א) ", a daf + * label) shifts on a neighbour's insert and would churn ids again. */ - fun lineNaturalKeyHash(content: String, heRef: String?): ByteArray { - val prefixed = if (heRef != null) "REF:$heRef" else "CT:$content" - return MessageDigest.getInstance("SHA-1").digest(prefixed.toByteArray(Charsets.UTF_8)) - } + fun lineNaturalKeyHash(content: String): ByteArray = + MessageDigest.getInstance("SHA-1").digest("CT:$content".toByteArray(Charsets.UTF_8)) /** * sha1 of the line content, used as part of the natural key for `line`. diff --git a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/InMemoryIdAllocator.kt b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/InMemoryIdAllocator.kt index 1ec1377e..181d519b 100644 --- a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/InMemoryIdAllocator.kt +++ b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/InMemoryIdAllocator.kt @@ -25,8 +25,6 @@ import java.util.concurrent.atomic.AtomicLong * - Fresh allocations come from a per-table [AtomicLong] counter. * - Counters start at `max(previous next_id, previous max(id) + 1, 1)` so we * never collide with reused ids even if the previous snapshot was incomplete. - * - * See DELTA_UPDATE_PLAN.md §3.5. */ class InMemoryIdAllocator private constructor( previous: BuildStateSnapshot, @@ -97,6 +95,33 @@ class InMemoryIdAllocator private constructor( private val previousMeta: Map = previous.meta + /** Line keys the seed build_state carried in — 0 means there was nothing to migrate. */ + private val seedLineCount: Int = previous.lines.size + + // The shim reads the seed only, never the live `lines` map: a line processed + // earlier this build may already have moved or claimed the id it would find. + private val seedLines: Map = previous.lines + + // id -> key issued this build. One id per line is the invariant this guards. + private val issuedLineIds = ConcurrentHashMap() + + // Transition shim counters (see LegacyLineKey); reported by snapshotTo. + private val legacyLineKeysMigrated = AtomicLong(0) + private val legacyLineKeyLookups = AtomicLong(0) + private val legacyLineKeyCollisions = AtomicLong(0) + + /** How many line ids were carried over from a pre-#1211 build_state. */ + fun legacyLineKeysMigrated(): Long = legacyLineKeysMigrated.get() + + /** Lines whose legacy key differed from the new one, so the shim was consulted. */ + fun legacyLineKeyLookups(): Long = legacyLineKeyLookups.get() + + /** Lines the shim was consulted for and could not resolve — they got a fresh id. */ + fun legacyLineKeysMissed(): Long = legacyLineKeyLookups.get() - legacyLineKeysMigrated.get() + + /** Legacy hits whose seed id another line had already taken this build — given a fresh id. */ + fun legacyLineKeyCollisions(): Long = legacyLineKeyCollisions.get() + // ─── Lookup-table accessors ──────────────────────────────────────────────── private fun allocateLookup(table: IdTable, key: String): Long { @@ -150,17 +175,66 @@ class InMemoryIdAllocator private constructor( } } - override fun lineId(bookId: Long, contentHash: ByteArray, occurrenceIdx: Int): Long { + override fun lineId(bookId: Long, contentHash: ByteArray, occurrenceIdx: Int): Long = + lineId(bookId, contentHash, occurrenceIdx, legacy = null) + + override fun lineId( + bookId: Long, + contentHash: ByteArray, + occurrenceIdx: Int, + legacy: LegacyLineKey?, + ): Long { require(contentHash.size == 20) { "contentHash must be 20-byte sha1, got ${contentHash.size}" } val key = LineKey(bookId, contentHash, occurrenceIdx) lines[key]?.let { + reusedCount.getValue(IdTable.LINE).incrementAndGet() + return claimLineId(key, it) + } + migrateLegacyLineKey(bookId, key, legacy)?.let { reusedCount.getValue(IdTable.LINE).incrementAndGet() return it } - return lines.computeIfAbsent(key) { + val fresh = lines.computeIfAbsent(key) { freshCount.getValue(IdTable.LINE).incrementAndGet() counters.getValue(IdTable.LINE).getAndIncrement() } + return claimLineId(key, fresh) + } + + /** Records that [id] belongs to [key] this build; a second key for the same id is a corrupt seed. */ + private fun claimLineId(key: LineKey, id: Long): Long { + val holder = issuedLineIds.putIfAbsent(id, key) ?: return id + check(holder == key) { "line id $id is filed under two keys: $holder and $key" } + return id + } + + /** + * Transition shim (see [LegacyLineKey]): moves the id a pre-#1211 build + * filed under the heRef-based key over to [newKey]. Tried for every line, + * not only ref-bearing ones — a generated prefix moved into the old key too. + */ + private fun migrateLegacyLineKey(bookId: Long, newKey: LineKey, legacy: LegacyLineKey?): Long? { + if (legacy == null) return null + val legacyKey = LineKey(bookId, legacy.contentHash, legacy.occurrenceIdx) + if (legacyKey == newKey) return null + legacyLineKeyLookups.incrementAndGet() + val id = seedLines[legacyKey] ?: return null + // The seed id may already be live under a different new key (same content, a + // shifted occurrence index). Handing it out twice would drop a line on insert. + val holder = issuedLineIds.putIfAbsent(id, newKey) + if (holder != null && holder != newKey) { + legacyLineKeyCollisions.incrementAndGet() + return null + } + legacyLineKeysMigrated.incrementAndGet() + lines.remove(legacyKey, id) + val existing = lines.putIfAbsent(newKey, id) + if (existing != null && existing != id) { + // Raced with a direct hit on newKey: yield to it and release our claim. + issuedLineIds.remove(id, newKey) + return claimLineId(newKey, existing) + } + return id } override fun tocEntryId(bookId: Long, ancestorPath: String): Long { @@ -253,6 +327,12 @@ class InMemoryIdAllocator private constructor( // future re-add would be classified as `added`, not `unchanged`. val liveBookIds = books.values.toHashSet() val gcLines = lines.entries.removeIfMatches { it.key.bookId !in liveBookIds } + // A book processed this build allocated every line it has; any other key of that + // book is a leftover (a legacy key whose id went elsewhere) and must not be reseeded. + val touchedBookIds = issuedLineIds.values.mapTo(HashSet()) { it.bookId } + val gcStaleLineKeys = lines.entries.removeIfMatches { + it.key.bookId in touchedBookIds && issuedLineIds[it.value] != it.key + } val gcTocs = tocEntries.entries.removeIfMatches { it.key.bookId !in liveBookIds } val gcAltStructs = altTocStructures.entries.removeIfMatches { it.key.bookId !in liveBookIds } val liveStructureIds = altTocStructures.values.toHashSet() @@ -263,10 +343,10 @@ class InMemoryIdAllocator private constructor( } val liveBookKeys = books.keys.toHashSet() val gcSourceHashes = mergedSourceHashes.entries.removeIfMatches { it.key !in liveBookKeys } - if (gcLines + gcTocs + gcAltStructs + gcAltEntries + gcLinks + gcSourceHashes > 0) { + if (gcLines + gcStaleLineKeys + gcTocs + gcAltStructs + gcAltEntries + gcLinks + gcSourceHashes > 0) { logger.i { "Phase-8 GC pruned orphan entries: " + - "lines=$gcLines, tocEntries=$gcTocs, " + + "lines=$gcLines, staleLineKeys=$gcStaleLineKeys, tocEntries=$gcTocs, " + "altStructures=$gcAltStructs, altEntries=$gcAltEntries, " + "links=$gcLinks, sourceHashes=$gcSourceHashes" } @@ -286,6 +366,7 @@ class InMemoryIdAllocator private constructor( bookAliases = bookAliases.values.toList(), sourceHashes = mergedSourceHashes, ) + logLegacyLineKeyTransition() BuildStateWriter(logger).write(snapshot, target) val stats = stats() logger.i { @@ -296,6 +377,29 @@ class InMemoryIdAllocator private constructor( } } + /** + * Build-summary line for the #1211 key change: a transition build that did + * NOT carry its ids over is the failure mode worth seeing, so it warns. + */ + private fun logLegacyLineKeyTransition() { + val lookups = legacyLineKeyLookups.get() + if (seedLineCount == 0 || lookups == 0L) return + val migrated = legacyLineKeysMigrated.get() + val missed = lookups - migrated + logger.i { + "Legacy line-key transition: seed held $seedLineCount line keys; " + + "$lookups lines re-keyed, $migrated migrated, $missed given fresh ids " + + "(${legacyLineKeyCollisions.get()} of them because the seed id was already taken)" + } + if (missed > lookups * LEGACY_MISS_WARN_FRACTION) { + logger.w { + "Legacy line-key transition looks unclean: $missed of $lookups re-keyed lines " + + "(${"%.1f".format(missed * 100.0 / lookups)}%) got a fresh id instead of the seed's — " + + "expect a large delta for this release" + } + } + } + /** * Removes entries matching [predicate] from this mutable entry set and * returns how many it removed. Avoids the explicit double-iteration @@ -315,6 +419,9 @@ class InMemoryIdAllocator private constructor( } companion object { + /** Above this share of re-keyed lines missing from the seed, the transition warns. */ + private const val LEGACY_MISS_WARN_FRACTION = 0.01 + /** Loads a previous build_state from [path] (empty if missing) and returns an allocator. */ fun load(path: Path?, logger: Logger = Logger.withTag("IdAllocator")): InMemoryIdAllocator { val previous = if (path == null) { diff --git a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LegacyLineKey.kt b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LegacyLineKey.kt new file mode 100644 index 00000000..9d97396b --- /dev/null +++ b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LegacyLineKey.kt @@ -0,0 +1,26 @@ +package io.github.kdroidfilter.seforimlibrary.common.ids + +import java.security.MessageDigest + +/** + * Transition shim. Builds up to and including db v27 keyed a Sefaria line on + * its heRef when it had one and on its rendered content otherwise, so both an + * heRef edit and a generated-prefix shift renumbered the book (issue #1211). + * + * The key is the raw segment now, so the fallback is tried for EVERY line, not + * only ref-bearing ones. A seed build_state written by an older build + * still holds the heRef-based keys, so [InMemoryIdAllocator.lineId] falls back + * to this key once and re-registers the id under the new one — the snapshot it + * writes is fully migrated. Delete this file (and the `legacy` parameter on + * [IdAllocator.lineId]) once no build_state in circulation predates the change. + */ +class LegacyLineKey(val contentHash: ByteArray, val occurrenceIdx: Int) { + + companion object { + /** The pre-#1211 natural-key hash: heRef when present, content otherwise. */ + fun hash(content: String, heRef: String?): ByteArray { + val prefixed = if (heRef != null) "REF:$heRef" else "CT:$content" + return MessageDigest.getInstance("SHA-1").digest(prefixed.toByteArray(Charsets.UTF_8)) + } + } +} diff --git a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LineOccurrenceCounter.kt b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LineOccurrenceCounter.kt new file mode 100644 index 00000000..ca3891cc --- /dev/null +++ b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LineOccurrenceCounter.kt @@ -0,0 +1,23 @@ +package io.github.kdroidfilter.seforimlibrary.common.ids + +import java.util.concurrent.ConcurrentHashMap + +/** + * Per-(bookId, contentHash) counter that gives duplicate lines inside one book + * distinct occurrence indices, so their natural keys — and thus ids — differ. + * + * One instance per key scheme per build: the index only means anything relative + * to the hash it counts. + */ +class LineOccurrenceCounter { + + private val byBook = ConcurrentHashMap>() + + fun next(bookId: Long, contentHash: ByteArray): Int { + // Lossy 64-bit fold: collisions inside a single book are vanishingly + // rare and only ever cost an occurrence index, never correctness. + val hashKey = contentHash.fold(0L) { acc, b -> (acc shl 5) - acc + b.toLong() } + val map = byBook.computeIfAbsent(bookId) { ConcurrentHashMap() } + return map.compute(hashKey) { _, v -> (v ?: -1) + 1 }!! + } +} diff --git a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchPipelineCli.kt b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchPipelineCli.kt index 73d25fd5..05a6b2d1 100644 --- a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchPipelineCli.kt +++ b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchPipelineCli.kt @@ -23,9 +23,10 @@ import kotlin.system.exitProcess * * Exit codes: * - `0` patch produced and verified. - * - [UnpatchableAnchorException.EXIT_CODE] (3) the (prev, new) pair cannot - * be expressed as a delta at all (missing PK column, or a column dropped - * without a `db_schema_version` bump). A `.unpatchable` marker file + * - [UnpatchableAnchorException.EXIT_CODE] (3) this anchor must not ship a + * delta: either the pair cannot be expressed as one at all (missing PK + * column, or a column dropped without a `db_schema_version` bump), or the + * delta is oversized (see [PatchSizeGuard]). A `.unpatchable` marker file * carrying the reason is written next to the patch, the Gradle task lets * the build succeed, and the release workflow skips just that anchor. * - any other non-zero: a genuine failure (hash mismatch, IO, …) — the @@ -88,12 +89,36 @@ fun main(args: Array) { } exitProcess(UnpatchableAnchorException.EXIT_CODE) } + val totalUpserts = output.upsertCounts.values.sum() val totalDeletes = output.deleteCounts.values.sum() + // Logged before the size guard: on a skip these counts are the only clue to + // WHY the delta blew up (id churn shows as an upsert per line). logger.i { "patch.db produced — upserts=$totalUpserts, deletes=$totalDeletes" } logger.i { " upserts by table: ${output.upsertCounts.filterValues { it > 0 }}" } logger.i { " deletes by table: ${output.deleteCounts.filterValues { it > 0 }}" } + // Oversized-delta guard. Runs before verify/compress: an anchor we will not + // publish should not burn the apply-and-hash pass either. + val sizeDecision = PatchSizeGuard.decide( + patchUncompressedSize = Files.size(outPath), + newDbSize = Files.size(newPath), + maxRatio = PatchSizeGuard.configuredMaxRatio(), + ) + if (!sizeDecision.publish) { + // Leading token, not prose: the release workflow tells a size-guard skip + // (still a publishable full-only release) from a structural one by it. + val reason = "${PatchSizeGuard.MARKER_REASON_TOKEN}: anchor v$from → v$to produced an oversized delta: " + + "${sizeDecision.describe()}, upserts=$totalUpserts deletes=$totalDeletes — " + + "a full-bundle download is faster for the client than applying it" + Files.createDirectories(unpatchableMarker.toAbsolutePath().parent) + Files.write(unpatchableMarker, "$reason\n".toByteArray(Charsets.UTF_8)) + runCatching { Files.deleteIfExists(outPath) } + logger.w { "$reason — wrote $unpatchableMarker and exiting ${UnpatchableAnchorException.EXIT_CODE}" } + exitProcess(UnpatchableAnchorException.EXIT_CODE) + } + logger.i { "Patch size within budget: ${sizeDecision.describe()}" } + // Verify apply: copy prev, apply patch, hash, compare with hash(new). val target = outPath.resolveSibling("verify-${outPath.fileName}") Files.copy(prevPath, target, StandardCopyOption.REPLACE_EXISTING) diff --git a/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchSizeGuard.kt b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchSizeGuard.kt new file mode 100644 index 00000000..20713748 --- /dev/null +++ b/generator/common/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchSizeGuard.kt @@ -0,0 +1,61 @@ +package io.github.kdroidfilter.seforimlibrary.common.patch + +/** + * Hard safety net against shipping a pathological delta (Otzaria issue #1211): + * clients that cannot offer the user a choice would otherwise spend far longer + * applying indexed upserts than downloading the full DB. + * + * Deliberately looser than the Otzaria updater's own 0.25 "heavy delta" mark — + * that one only labels the route so the user can still prefer the smaller + * download on a slow link; this one removes the route altogether. + */ +object PatchSizeGuard { + + // v27's pathological patches were 31.7%–40.3% of the new DB, so 0.5 + // would have allowed the exact incident this guard exists to prevent. + const val DEFAULT_MAX_DELTA_UNCOMPRESSED_RATIO: Double = 0.30 + + /** First token of the `.unpatchable` marker written when this guard fires. */ + const val MARKER_REASON_TOKEN: String = "oversized-delta" + + /** System property / env var that overrides [DEFAULT_MAX_DELTA_UNCOMPRESSED_RATIO]. */ + const val RATIO_PROPERTY: String = "maxDeltaUncompressedRatio" + const val RATIO_ENV: String = "MAX_DELTA_UNCOMPRESSED_RATIO" + + data class Decision( + val publish: Boolean, + val patchUncompressedSize: Long, + val newDbSize: Long, + val ratio: Double, + val maxRatio: Double, + ) { + fun describe(): String = + "patch.db ${patchUncompressedSize} B is ${"%.1f".format(ratio * 100)}% of the new seforim.db " + + "($newDbSize B); limit is ${"%.1f".format(maxRatio * 100)}%" + } + + fun decide( + patchUncompressedSize: Long, + newDbSize: Long, + maxRatio: Double = DEFAULT_MAX_DELTA_UNCOMPRESSED_RATIO, + ): Decision { + require(maxRatio > 0.0) { "maxDeltaUncompressedRatio must be positive, got $maxRatio" } + require(newDbSize > 0L) { "new seforim.db size must be positive, got $newDbSize" } + require(patchUncompressedSize >= 0L) { "patch size must not be negative, got $patchUncompressedSize" } + val ratio = patchUncompressedSize.toDouble() / newDbSize.toDouble() + return Decision( + publish = ratio <= maxRatio, + patchUncompressedSize = patchUncompressedSize, + newDbSize = newDbSize, + ratio = ratio, + maxRatio = maxRatio, + ) + } + + /** Reads the configured limit: `-D`/`-P` first, then env, else the default. */ + fun configuredMaxRatio(): Double { + val raw = System.getProperty(RATIO_PROPERTY) ?: System.getenv(RATIO_ENV) + if (raw == null) return DEFAULT_MAX_DELTA_UNCOMPRESSED_RATIO + return raw.toDoubleOrNull() ?: error("$RATIO_PROPERTY='$raw' is not a number") + } +} diff --git a/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LegacyLineKeyMigrationTest.kt b/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LegacyLineKeyMigrationTest.kt new file mode 100644 index 00000000..93ee5c7e --- /dev/null +++ b/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LegacyLineKeyMigrationTest.kt @@ -0,0 +1,205 @@ +package io.github.kdroidfilter.seforimlibrary.common.ids + +import io.github.kdroidfilter.seforimlibrary.common.buildstate.BuildStateReader +import io.github.kdroidfilter.seforimlibrary.common.buildstate.BuildStateSnapshot +import io.github.kdroidfilter.seforimlibrary.common.buildstate.LineKey +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNotEquals +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Issue #1211: line ids must survive an heRef-only edit, and a build_state + * written under the old heRef-based key must seed the new scheme in place. + */ +class LegacyLineKeyMigrationTest { + + @JvmField @Rule + val tmp = TemporaryFolder() + + private val content = listOf("פסוק א", "פסוק ב", "פסוק ג") + + /** Runs one Sefaria-shaped book through the allocator with the given heRefs. */ + private fun allocate( + allocator: InMemoryIdAllocator, + bookId: Long, + lines: List, + heRefs: List, + ): List { + val occ = LineOccurrenceCounter() + val legacyOcc = LineOccurrenceCounter() + return lines.mapIndexed { idx, text -> + val hash = IdAllocatorBindings.lineNaturalKeyHash(text) + val legacyHash = LegacyLineKey.hash(text, heRefs[idx]) + allocator.lineId( + bookId, + hash, + occ.next(bookId, hash), + LegacyLineKey(legacyHash, legacyOcc.next(bookId, legacyHash)), + ) + } + } + + @Test + fun `same content with a changed heRef keeps its id across builds`() { + val statePath = tmp.newFolder().toPath().resolve("build_state.db") + val build1 = InMemoryIdAllocator.load(null) + val bookId = build1.bookId("Sefaria", "בראשית") + val ids1 = allocate(build1, bookId, content, listOf("בראשית א׳:א׳", "בראשית א׳:ב׳", "בראשית א׳:ג׳")) + build1.snapshotTo(statePath) + + // Commit 4e286b9's shape: a comma inserted into every heRef. + val build2 = InMemoryIdAllocator.load(statePath) + val ids2 = allocate( + build2, + build2.bookId("Sefaria", "בראשית"), + content, + listOf("בראשית, א׳:א׳", "בראשית, א׳:ב׳", "בראשית, א׳:ג׳"), + ) + assertEquals(ids1, ids2, "an heRef edit must not renumber lines") + } + + @Test + fun `a legacy snapshot seeds the new scheme with identical ids`() { + val statePath = tmp.newFolder().toPath().resolve("build_state.db") + val heRefs = listOf("בראשית א׳:א׳", "בראשית א׳:ב׳", null) + + // Build 1 emulates a pre-#1211 generator: keys carry the heRef hash. + val build1 = InMemoryIdAllocator.load(null) + val bookId = build1.bookId("Sefaria", "בראשית") + val legacyOcc = LineOccurrenceCounter() + val legacyIds = content.mapIndexed { idx, text -> + val h = LegacyLineKey.hash(text, heRefs[idx]) + build1.lineId(bookId, h, legacyOcc.next(bookId, h)) + } + build1.snapshotTo(statePath) + val seeded = BuildStateReader().read(statePath) + assertEquals(3, seeded.lines.size) + + // Build 2 uses the new key and must reuse every id via the shim. + val build2 = InMemoryIdAllocator.load(statePath) + val newIds = allocate(build2, build2.bookId("Sefaria", "בראשית"), content, heRefs) + assertEquals(legacyIds, newIds) + assertEquals(2, build2.legacyLineKeysMigrated(), "the heRef-less line needed no migration") + + // The written snapshot is fully migrated: new keys only, one per id. + val statePath2 = tmp.newFolder().toPath().resolve("build_state.db") + build2.snapshotTo(statePath2) + val migrated = BuildStateReader().read(statePath2) + assertEquals(3, migrated.lines.size) + assertEquals(newIds.size, migrated.lines.values.toSet().size, "no id may sit under two keys") + content.forEachIndexed { idx, text -> + val key = LineKey(bookId, IdAllocatorBindings.lineNaturalKeyHash(text), 0) + assertEquals(newIds[idx], migrated.lines[key]) + } + heRefs.filterNotNull().forEach { ref -> + assertNull( + migrated.lines[LineKey(bookId, LegacyLineKey.hash(content[0], ref), 0)], + "legacy key $ref must be gone from the written snapshot", + ) + } + + // Build 3 sees no legacy keys and still resolves the same ids. + val build3 = InMemoryIdAllocator.load(statePath2) + assertEquals(newIds, allocate(build3, build3.bookId("Sefaria", "בראשית"), content, heRefs)) + assertEquals(0, build3.legacyLineKeysMigrated()) + } + + @Test + fun `duplicate content lines get distinct occurrence ids`() { + val allocator = InMemoryIdAllocator.load(null) + val bookId = allocator.bookId("Sefaria", "ספר") + val ids = allocate( + allocator, + bookId, + listOf("אמן", "אמן", "אמן"), + listOf("ר׳ א", "ר׳ ב", "ר׳ ג"), + ) + assertEquals(3, ids.toSet().size, "identical content must still get distinct ids") + } + + /** + * Review finding on PR #28: line A (heRef, content X) and line B (no heRef, content X) + * were seeded as `REF:A#0` and `CT:X#0`. Under the new key A takes `CT:X#0` — B's old + * id — and B's legacy key is that very `CT:X#0`; the shim must not hand the id out twice. + */ + @Test + fun `legacy hit on an id another line already took this build gets a fresh id`() { + val statePath = tmp.newFolder().toPath().resolve("build_state.db") + val lines = listOf("אמן", "אמן") + val heRefs = listOf("ר׳ א", null) + + val build1 = InMemoryIdAllocator.load(null) + val bookId = build1.bookId("Sefaria", "ספר") + val legacyOcc = LineOccurrenceCounter() + val legacyIds = lines.mapIndexed { idx, text -> + val h = LegacyLineKey.hash(text, heRefs[idx]) + build1.lineId(bookId, h, legacyOcc.next(bookId, h)) + } + assertNotEquals(legacyIds[0], legacyIds[1]) + build1.snapshotTo(statePath) + + val build2 = InMemoryIdAllocator.load(statePath) + val newIds = allocate(build2, build2.bookId("Sefaria", "ספר"), lines, heRefs) + assertEquals(2, newIds.toSet().size, "two lines must never share an id") + assertEquals(legacyIds[1], newIds[0], "A now owns CT:X#0 and inherits B's old id") + assertEquals(1, build2.legacyLineKeyCollisions()) + assertEquals(0, build2.legacyLineKeysMigrated()) + + val statePath2 = tmp.newFolder().toPath().resolve("build_state.db") + build2.snapshotTo(statePath2) + val migrated = BuildStateReader().read(statePath2) + assertEquals(2, migrated.lines.size) + assertEquals(2, migrated.lines.values.toSet().size, "no id may sit under two keys") + + // The next build is stable: same input, same ids, nothing left to migrate. + val build3 = InMemoryIdAllocator.load(statePath2) + assertEquals(newIds, allocate(build3, build3.bookId("Sefaria", "ספר"), lines, heRefs)) + assertEquals(0, build3.legacyLineKeyCollisions()) + } + + @Test + fun `a seed that files one id under two keys fails the build instead of dropping a line`() { + val bookId = 7L + val hashA = IdAllocatorBindings.lineNaturalKeyHash("א") + val hashB = IdAllocatorBindings.lineNaturalKeyHash("ב") + val corrupt = BuildStateSnapshot.empty().copy( + lines = mapOf(LineKey(bookId, hashA, 0) to 5L, LineKey(bookId, hashB, 0) to 5L), + ) + val allocator = InMemoryIdAllocator.fromSnapshot(corrupt) + allocator.lineId(bookId, hashA, 0) + assertFailsWith { allocator.lineId(bookId, hashB, 0) } + } + + @Test + fun `duplicate content under legacy distinct refs migrates one id per line`() { + val statePath = tmp.newFolder().toPath().resolve("build_state.db") + val lines = listOf("אמן", "אמן") + val heRefs = listOf("ר׳ א", "ר׳ ב") + + val build1 = InMemoryIdAllocator.load(null) + val bookId = build1.bookId("Sefaria", "ספר") + val legacyOcc = LineOccurrenceCounter() + val legacyIds = lines.mapIndexed { idx, text -> + val h = LegacyLineKey.hash(text, heRefs[idx]) + build1.lineId(bookId, h, legacyOcc.next(bookId, h)) + } + assertNotEquals(legacyIds[0], legacyIds[1]) + build1.snapshotTo(statePath) + + val build2 = InMemoryIdAllocator.load(statePath) + val newIds = allocate(build2, build2.bookId("Sefaria", "ספר"), lines, heRefs) + assertEquals(legacyIds, newIds, "occurrence order must map legacy keys onto new ones 1:1") + assertEquals(2, build2.legacyLineKeysMigrated()) + + val statePath2 = tmp.newFolder().toPath().resolve("build_state.db") + build2.snapshotTo(statePath2) + val migrated = BuildStateReader().read(statePath2) + assertEquals(2, migrated.lines.size) + assertTrue(migrated.lines.values.containsAll(newIds)) + } +} diff --git a/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LineNaturalKeyHashTest.kt b/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LineNaturalKeyHashTest.kt index cb422634..b85234b0 100644 --- a/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LineNaturalKeyHashTest.kt +++ b/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/ids/LineNaturalKeyHashTest.kt @@ -8,65 +8,49 @@ import kotlin.test.assertNotEquals class LineNaturalKeyHashTest { @Test - fun `heRef key is stable across content reformatting`() { - // Same heRef, different rendered prefix: should yield the same hash. - val a = IdAllocatorBindings.lineNaturalKeyHash( - content = "(א) בְּרֵאשִׁית בָּרָא אֱלֹהִים", - heRef = "Genesis 1:1", + fun `hash ignores heRef entirely`() { + // The v27 regression (issue #1211): a heRef-only edit must not move the key. + val content = "בְּרֵאשִׁית בָּרָא אֱלֹהִים" + assertEquals( + IdAllocatorBindings.lineNaturalKeyHash(content).toList(), + IdAllocatorBindings.lineNaturalKeyHash(content).toList(), ) - val b = IdAllocatorBindings.lineNaturalKeyHash( - content = "(ב) בְּרֵאשִׁית בָּרָא אֱלֹהִים", // prefix shifted by an insertion - heRef = "Genesis 1:1", // citation unchanged + assertNotEquals( + LegacyLineKey.hash(content, "בראשית א׳:א׳").toList(), + LegacyLineKey.hash(content, "בראשית, א׳:א׳").toList(), + "the legacy key was heRef-sensitive — that is what this change removes", ) - assertEquals(a.toList(), b.toList(), "heRef-keyed hash must ignore rendered content") } @Test - fun `distinct heRef produces distinct hash`() { - val verse1 = IdAllocatorBindings.lineNaturalKeyHash("foo", "Genesis 1:1") - val verse2 = IdAllocatorBindings.lineNaturalKeyHash("foo", "Genesis 1:2") - assertNotEquals(verse1.toList(), verse2.toList()) - } - - @Test - fun `content-hash fallback when heRef is null`() { - val headingA = IdAllocatorBindings.lineNaturalKeyHash("

בראשית

", null) - val headingB = IdAllocatorBindings.lineNaturalKeyHash("

בראשית

", null) - assertEquals(headingA.toList(), headingB.toList()) - - val differentContent = IdAllocatorBindings.lineNaturalKeyHash("

פרק א

", null) - assertNotEquals(headingA.toList(), differentContent.toList()) - } - - @Test - fun `ref and content namespaces don't collide`() { - // If "REF:foo" and "CT:foo" both hashed naively, they could collide. - // The discriminator prefix prevents that. - val viaRef = IdAllocatorBindings.lineNaturalKeyHash("anything", "foo") - val viaContent = IdAllocatorBindings.lineNaturalKeyHash("foo", null) - assertNotEquals(viaRef.toList(), viaContent.toList()) + fun `distinct content produces distinct hash`() { + assertNotEquals( + IdAllocatorBindings.lineNaturalKeyHash("

בראשית

").toList(), + IdAllocatorBindings.lineNaturalKeyHash("

פרק א

").toList(), + ) } @Test fun `hash is always 20 bytes`() { - assertEquals(20, IdAllocatorBindings.lineNaturalKeyHash("x", "y").size) - assertEquals(20, IdAllocatorBindings.lineNaturalKeyHash("x", null).size) + assertEquals(20, IdAllocatorBindings.lineNaturalKeyHash("x").size) + assertEquals(20, IdAllocatorBindings.lineNaturalKeyHash("").size) + assertEquals(20, LegacyLineKey.hash("x", "y").size) } @Test - fun `empty content with heRef still keyed by heRef`() { - val a = IdAllocatorBindings.lineNaturalKeyHash("", "Genesis 1:1") - val b = IdAllocatorBindings.lineNaturalKeyHash("some text", "Genesis 1:1") - assertEquals(a.toList(), b.toList()) + fun `heading lines keep the pre-change key`() { + // Lines without an heRef were already keyed on "CT:"; keeping that + // shape means the change churns only the ref-bearing lines. + assertEquals( + LegacyLineKey.hash("

בראשית

", null).toList(), + IdAllocatorBindings.lineNaturalKeyHash("

בראשית

").toList(), + ) } @Test - fun `Otzaria-style raw content path is byte-stable across calls`() { - val a = IdAllocatorBindings.lineNaturalKeyHash("

שלום

", null) - val b = IdAllocatorBindings.lineNaturalKeyHash("

שלום

", null) - assertEquals(a.toList(), b.toList()) - // And differs from the legacy normalisedContentHash (which doesn't prefix). - val legacy = IdAllocatorBindings.normalisedContentHash("

שלום

") - assertFalse(a.toList() == legacy.toList(), "new hash must be namespaced (CT: prefix)") + fun `namespaced apart from the Otzaria raw content hash`() { + val a = IdAllocatorBindings.lineNaturalKeyHash("

שלום

") + val raw = IdAllocatorBindings.normalisedContentHash("

שלום

") + assertFalse(a.toList() == raw.toList(), "new hash must stay namespaced (CT: prefix)") } } diff --git a/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchSizeGuardTest.kt b/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchSizeGuardTest.kt new file mode 100644 index 00000000..faee2c33 --- /dev/null +++ b/generator/common/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/common/patch/PatchSizeGuardTest.kt @@ -0,0 +1,86 @@ +package io.github.kdroidfilter.seforimlibrary.common.patch + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class PatchSizeGuardTest { + + private val dbSize = 4_000_000_000L + + @Test + fun `small delta publishes`() { + val d = PatchSizeGuard.decide(patchUncompressedSize = 40_000_000L, newDbSize = dbSize) + assertTrue(d.publish) + assertEquals(0.01, d.ratio, 1e-9) + } + + @Test + fun `the v27 shape is rejected`() { + // The real v27 fan ranged from 31.7% to 40.3% of the new DB. + val d = PatchSizeGuard.decide(patchUncompressedSize = 1_268_000_000L, newDbSize = dbSize) + assertFalse(d.publish) + assertTrue(d.describe().contains("limit is 30.0%")) + } + + @Test + fun `exactly at the limit still publishes`() { + val d = PatchSizeGuard.decide(patchUncompressedSize = 1_200_000_000L, newDbSize = dbSize) + assertTrue(d.publish) + assertEquals(PatchSizeGuard.DEFAULT_MAX_DELTA_UNCOMPRESSED_RATIO, d.maxRatio) + } + + @Test + fun `one byte over the limit is rejected`() { + assertFalse(PatchSizeGuard.decide(1_200_000_001L, dbSize).publish) + } + + @Test + fun `the default is the server-side safety net, looser than the updater's heavy mark`() { + assertEquals(0.30, PatchSizeGuard.DEFAULT_MAX_DELTA_UNCOMPRESSED_RATIO) + // The updater calls 0.25 "heavy" and lets the user choose; the guard + // only removes deltas no client should ever be handed. + assertTrue(PatchSizeGuard.decide(1_100_000_000L, dbSize).publish) + } + + @Test + fun `an explicit ratio overrides the default`() { + assertTrue(PatchSizeGuard.decide(3_000_000_000L, dbSize, maxRatio = 0.9).publish) + assertFalse(PatchSizeGuard.decide(40_000_000L, dbSize, maxRatio = 0.001).publish) + } + + @Test + fun `an empty patch publishes`() { + assertTrue(PatchSizeGuard.decide(0L, dbSize).publish) + } + + @Test + fun `nonsensical inputs fail loudly`() { + assertFailsWith { PatchSizeGuard.decide(1L, 0L) } + assertFailsWith { PatchSizeGuard.decide(-1L, dbSize) } + assertFailsWith { PatchSizeGuard.decide(1L, dbSize, maxRatio = 0.0) } + } + + @Test + fun `configured ratio falls back to the default`() { + val previous = System.getProperty(PatchSizeGuard.RATIO_PROPERTY) + try { + System.clearProperty(PatchSizeGuard.RATIO_PROPERTY) + if (System.getenv(PatchSizeGuard.RATIO_ENV) == null) { + assertEquals( + PatchSizeGuard.DEFAULT_MAX_DELTA_UNCOMPRESSED_RATIO, + PatchSizeGuard.configuredMaxRatio(), + ) + } + System.setProperty(PatchSizeGuard.RATIO_PROPERTY, "0.25") + assertEquals(0.25, PatchSizeGuard.configuredMaxRatio()) + System.setProperty(PatchSizeGuard.RATIO_PROPERTY, "not-a-number") + assertFailsWith { PatchSizeGuard.configuredMaxRatio() } + } finally { + if (previous == null) System.clearProperty(PatchSizeGuard.RATIO_PROPERTY) + else System.setProperty(PatchSizeGuard.RATIO_PROPERTY, previous) + } + } +} diff --git a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaBookPayloadReader.kt b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaBookPayloadReader.kt index 60a7aa94..da80753f 100644 --- a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaBookPayloadReader.kt +++ b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaBookPayloadReader.kt @@ -673,7 +673,11 @@ internal class SefariaBookPayloadReader( output += linePrefix + cleaned if (cleanShifts != null) { if (cleaned != content) { - cleanShifts[output.size - 1] = CLEAN_MODIFIED + // Keep the char-offset gate and the generated-prefix + // length in one value: anchors reject every negative + // value, while the line-key path can still strip the + // prefix from cleaned content. + cleanShifts[output.size - 1] = cleanedLineShift(linePrefix.length) } else if (linePrefix.isNotEmpty()) { cleanShifts[output.size - 1] = linePrefix.length } diff --git a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchors.kt b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchors.kt index 2da17f2c..677bf221 100644 --- a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchors.kt +++ b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchors.kt @@ -59,7 +59,7 @@ internal class SefariaCharLevelAnchors( continue } val shift = book.cleanShiftByLineIndex[entry.lineIndex0] ?: 0 - if (shift == CLEAN_MODIFIED) { + if (lineWasModifiedByCleaning(shift)) { skip("line modified by cleaning") continue } diff --git a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaDirectImporter.kt b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaDirectImporter.kt index 32b66166..522a7199 100644 --- a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaDirectImporter.kt +++ b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaDirectImporter.kt @@ -7,6 +7,8 @@ import io.github.kdroidfilter.seforimlibrary.common.changes.TouchedBookDetector import io.github.kdroidfilter.seforimlibrary.common.ids.IdAllocator import io.github.kdroidfilter.seforimlibrary.common.ids.IdAllocatorBindings import io.github.kdroidfilter.seforimlibrary.common.ids.InMemoryIdAllocator +import io.github.kdroidfilter.seforimlibrary.common.ids.LegacyLineKey +import io.github.kdroidfilter.seforimlibrary.common.ids.LineOccurrenceCounter import io.github.kdroidfilter.seforimlibrary.common.reports.GeneratorReport import io.github.kdroidfilter.seforimlibrary.core.models.Author import io.github.kdroidfilter.seforimlibrary.core.models.Book @@ -198,14 +200,10 @@ class SefariaDirectImporter( // Per-(bookId, contentHash) occurrence counter, so identical lines within // the same book still receive distinct stable ids. - val lineOccurrenceByBook = ConcurrentHashMap>() - fun nextLineOccurrence(bookId: Long, contentHash: ByteArray): Int { - // contentHash key: lossy 64-bit hash is enough since collisions inside a - // single book are vanishingly rare; we only need a per-book counter. - val hashKey = contentHash.fold(0L) { acc, b -> (acc shl 5) - acc + b.toLong() } - val map = lineOccurrenceByBook.computeIfAbsent(bookId) { ConcurrentHashMap() } - return map.compute(hashKey) { _, v -> (v ?: -1) + 1 }!! - } + val lineOccurrences = LineOccurrenceCounter() + // Transition shim (see LegacyLineKey): the legacy scheme counted + // occurrences of content+heRef, so it needs its own counter. + val legacyLineOccurrences = LineOccurrenceCounter() val lineKeyToId = ConcurrentHashMap, Long>() val lineIdToBookId = ConcurrentHashMap() @@ -254,6 +252,8 @@ class SefariaDirectImporter( } val lineKeyHashes = precomputed.lineKeyHashes ?: error("Payload '${payload.heTitle}' precompute was already released") + val legacyLineKeyHashes = precomputed.legacyLineKeyHashes + ?: error("Payload '${payload.heTitle}' precompute was already released") val lineCharCounts = precomputed.lineCharCounts ?: error("Payload '${payload.heTitle}' precompute was already released") val lineIsHeading = precomputed.lineIsHeading @@ -369,14 +369,13 @@ class SefariaDirectImporter( payload.lines.forEachIndexed { idx, content -> val refEntry = refsByLineIndex[idx] - // Prefers Sefaria's stable citation address (heRef) as natural key - // when available — survives Sefaria's verse-prefix renumbering - // (DELTA_UPDATE_PLAN.md §2.1). Falls back to a content hash for - // headings / structural lines that have no heRef. Computed on the - // parse worker; the array is handed to the allocator as-is. + // Keyed on the raw segment only — neither heRef nor the generated + // prefix. Both hashes come from the parse worker, used as-is. val contentHash = lineKeyHashes[idx] - val occurrence = nextLineOccurrence(bookId, contentHash) - val lineId = allocator.lineId(bookId, contentHash, occurrence) + val occurrence = lineOccurrences.next(bookId, contentHash) + val legacyHash = legacyLineKeyHashes[idx] + val legacy = LegacyLineKey(legacyHash, legacyLineOccurrences.next(bookId, legacyHash)) + val lineId = allocator.lineId(bookId, contentHash, occurrence, legacy) val lineCharCount = lineCharCounts[idx] lineBatch += Line( id = lineId, diff --git a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaImportModels.kt b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaImportModels.kt index cc49d10c..66164d6f 100644 --- a/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaImportModels.kt +++ b/generator/sefariasqlite/src/jvmMain/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaImportModels.kt @@ -2,6 +2,7 @@ package io.github.kdroidfilter.seforimlibrary.sefariasqlite import io.github.kdroidfilter.seforimlibrary.common.countVisibleChars import io.github.kdroidfilter.seforimlibrary.common.ids.IdAllocatorBindings +import io.github.kdroidfilter.seforimlibrary.common.ids.LegacyLineKey import io.github.kdroidfilter.seforimlibrary.core.models.PubDate import kotlinx.serialization.Serializable @@ -49,11 +50,24 @@ internal data class BookMeta( val collectiveTitleEn: String? = null, ) -/// Marker value in [BookPayload.cleanShiftByLineIndex]: the line's stored -/// content differs from the raw Sefaria segment (cleanSefariaLine modified -/// it), so raw char offsets cannot be mapped exactly onto it. +/// Base encoded value in [BookPayload.cleanShiftByLineIndex] for a line whose +/// content was modified by cleanSefariaLine and has no generated prefix. +/// A modified line with a prefix of length N is encoded as `CLEAN_MODIFIED - N`. internal const val CLEAN_MODIFIED = -1 +/** Encodes "cleaned content, generated prefix of [prefixLength] UTF-16 chars". */ +internal fun cleanedLineShift(prefixLength: Int): Int { + require(prefixLength >= 0) { "prefixLength must not be negative: $prefixLength" } + return CLEAN_MODIFIED - prefixLength +} + +/** Negative shift values mean raw char offsets are no longer exact. */ +internal fun lineWasModifiedByCleaning(encodedShift: Int): Boolean = encodedShift < 0 + +/** Recovers the generated-prefix length without conflating it with cleaning. */ +internal fun generatedPrefixLength(encodedShift: Int): Int = + if (lineWasModifiedByCleaning(encodedShift)) -(encodedShift + 1) else encodedShift + internal data class BookPayload( val heTitle: String, val enTitle: String, @@ -96,8 +110,8 @@ internal data class BookPayload( val singleVersionTitle: String? = null, // Sparse per-line offset bookkeeping for charLevelData mapping: // absent -> stored content == raw segment, no prefix - // n >= 0 -> stored content == "" + raw - // CLEAN_MODIFIED -> cleanSefariaLine changed the content; offsets unusable + // n >= 0 -> unchanged segment behind a generated prefix of length n + // n < 0 -> cleaned segment; prefix length is -(n + 1), offsets unusable val cleanShiftByLineIndex: Map = emptyMap(), // All [versionTitle, versionSource] pairs from merged.json's `versions` array // (the versions that CONTRIBUTED to the merge). book_version metadata-only @@ -143,13 +157,18 @@ internal class LinePrecompute( val hasTeamim: Boolean, val hasNekudot: Boolean, lineKeyHashes: Array, + legacyLineKeyHashes: Array, lineCharCounts: IntArray, lineIsHeading: BooleanArray, ) { - /// Per line, `IdAllocatorBindings.lineNaturalKeyHash(content, heRef)`. + /// Per line, the natural-key hash of the raw segment (generated prefixes stripped). var lineKeyHashes: Array? = lineKeyHashes private set + /// Per line, `LegacyLineKey.hash(content, heRef)` — transition shim only. + var legacyLineKeyHashes: Array? = legacyLineKeyHashes + private set + /// Per line, `countVisibleChars(content)`. var lineCharCounts: IntArray? = lineCharCounts private set @@ -164,6 +183,7 @@ internal class LinePrecompute( fun release() { lineKeyHashes = null + legacyLineKeyHashes = null lineCharCounts = null lineIsHeading = null } @@ -185,11 +205,13 @@ internal fun BookPayload.precomputeLineData(): BookPayload { val refsByLineIndex = refEntries.associateBy { it.lineIndex - 1 } val count = lines.size val hashes = arrayOfNulls(count) + val legacyHashes = arrayOfNulls(count) val charCounts = IntArray(count) val isHeading = BooleanArray(count) for (idx in 0 until count) { val content = lines[idx] - hashes[idx] = IdAllocatorBindings.lineNaturalKeyHash(content, refsByLineIndex[idx]?.heRef) + hashes[idx] = IdAllocatorBindings.lineNaturalKeyHash(rawSegmentForKey(idx, content)) + legacyHashes[idx] = LegacyLineKey.hash(content, refsByLineIndex[idx]?.heRef) charCounts[idx] = countVisibleChars(content) isHeading[idx] = content.contains("

") || content.contains("

") || content.contains("

") || content.contains("

") @@ -201,12 +223,24 @@ internal fun BookPayload.precomputeLineData(): BookPayload { hasTeamim = teamim, hasNekudot = nekudot, lineKeyHashes = hashes as Array, + legacyLineKeyHashes = legacyHashes as Array, lineCharCounts = charCounts, lineIsHeading = isHeading, ) return this } +/** + * The line's text with the generated prefix (`(א) `, daf labels…) stripped off. + * Inserting one verse reprefixes every later line of the chapter, and hashing + * the prefixed text would renumber all of their ids (issue #1211). + */ +private fun BookPayload.rawSegmentForKey(lineIndex: Int, content: String): String { + val encodedShift = cleanShiftByLineIndex[lineIndex] ?: return content + val prefixLength = generatedPrefixLength(encodedShift) + return if (prefixLength in 1..content.length) content.substring(prefixLength) else content +} + internal data class VersionMeta( val title: String, val source: String?, diff --git a/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/LineKeyPrefixStabilityTest.kt b/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/LineKeyPrefixStabilityTest.kt new file mode 100644 index 00000000..f26060f2 --- /dev/null +++ b/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/LineKeyPrefixStabilityTest.kt @@ -0,0 +1,158 @@ +package io.github.kdroidfilter.seforimlibrary.sefariasqlite + +import co.touchlab.kermit.Logger +import io.github.kdroidfilter.seforimlibrary.common.ids.IdAllocatorBindings +import io.github.kdroidfilter.seforimlibrary.common.ids.InMemoryIdAllocator +import io.github.kdroidfilter.seforimlibrary.common.ids.LegacyLineKey +import io.github.kdroidfilter.seforimlibrary.common.ids.LineOccurrenceCounter +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonArray +import kotlinx.serialization.json.JsonPrimitive +import kotlinx.serialization.json.jsonObject +import java.nio.file.Files +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals +import kotlin.test.assertTrue + +/** + * Issue #1211, second half: the importer injects "(א) ", "(ב) "… into + * `line.content`, so hashing the stored text would renumber a whole chapter + * whenever one verse is inserted at its top. The key hashes the raw segment. + */ +class LineKeyPrefixStabilityTest { + + /** "(א) " — the generated prefix the reader records in cleanShiftByLineIndex. */ + private val prefixLen = 4 + + private fun chapter(verses: List, withRefs: Boolean = true): BookPayload { + val lines = verses.mapIndexed { idx, v -> "(${'א' + idx}) $v" } + return BookPayload( + heTitle = "ספר בדיקה", enTitle = "Test Book", categoriesHe = listOf("תנך"), + lines = lines, + refEntries = if (!withRefs) emptyList() else lines.indices.map { + RefEntry(ref = "Test 1:${it + 1}", heRef = "בדיקה א׳:${'א' + it}", path = "p", lineIndex = it + 1) + }, + headings = emptyList(), authors = emptyList(), + description = null, heShortDesc = null, pubDates = emptyList(), altStructures = emptyList(), + cleanShiftByLineIndex = lines.indices.associateWith { prefixLen }, + ).precomputeLineData() + } + + /** Allocates the book's line ids exactly the way the insert loop does. */ + private fun allocate(allocator: InMemoryIdAllocator, bookId: Long, payload: BookPayload): List { + val pre = requireNotNull(payload.precomputed) + val occ = LineOccurrenceCounter() + val legacyOcc = LineOccurrenceCounter() + return payload.lines.indices.map { idx -> + val hash = pre.lineKeyHashes!![idx] + val legacyHash = pre.legacyLineKeyHashes!![idx] + allocator.lineId( + bookId, hash, occ.next(bookId, hash), + LegacyLineKey(legacyHash, legacyOcc.next(bookId, legacyHash)), + ) + } + } + + @Test + fun `the natural key ignores the generated prefix`() { + val pre = requireNotNull(chapter(listOf("פסוק אלף")).precomputed) + assertContentEquals( + IdAllocatorBindings.lineNaturalKeyHash("פסוק אלף"), + pre.lineKeyHashes!![0], + "the key must hash the raw segment, not \"(א) פסוק אלף\"", + ) + // The legacy array is the pre-#1211 shape and still sees the rendered line. + assertContentEquals( + LegacyLineKey.hash("(א) פסוק אלף", "בדיקה א׳:א"), + pre.legacyLineKeyHashes!![0], + ) + } + + @Test + fun `inserting a verse at the top keeps the ids of every verse after it`() { + val allocator = InMemoryIdAllocator.load(null) + val bookId = allocator.bookId("Sefaria", "ספר בדיקה") + val before = allocate(allocator, bookId, chapter(listOf("פסוק אלף", "פסוק בית", "פסוק גימל"))) + + // Same three verses, now prefixed (ב)(ג)(ד) because one was inserted. + val after = allocate( + allocator, + bookId, + chapter(listOf("פסוק חדש", "פסוק אלף", "פסוק בית", "פסוק גימל")), + ) + assertEquals(before, after.drop(1), "a head-insert must not renumber the rest of the chapter") + assertEquals(4, after.toSet().size) + assertEquals(0, allocator.legacyLineKeysMigrated(), "no seed, so nothing to migrate") + } + + @Test + fun `hashing the rendered line would have renumbered them`() { + // Pins WHY the shift is stripped: the rendered text really does differ. + val v1 = chapter(listOf("פסוק אלף", "פסוק בית")).lines[1] + val v2 = chapter(listOf("פסוק חדש", "פסוק אלף", "פסוק בית")).lines[2] + assertNotEquals(v1, v2) + assertEquals("פסוק בית", v1.substring(prefixLen)) + assertEquals("פסוק בית", v2.substring(prefixLen)) + } + + @Test + fun `cleaned lines still ignore a generated prefix`() { + val json = Json { ignoreUnknownKeys = true } + val reader = SefariaBookPayloadReader(json, Logger.withTag("LineKeyPrefixStabilityTest")) + val schema = json.parseToJsonElement( + """{"depth":1,"sectionNames":["Paragraph"],"addressTypes":["String"]}""", + ).jsonObject + fun cleanedLineHash(segments: List): ByteArray { + val built = reader.walkTextWithSchema( + schemaObj = schema, + textElement = JsonArray(segments.map(::JsonPrimitive)), + bookHeTitle = "ספר בדיקה", + bookEnTitle = "Test Book", + ) + val lineIndex = built.lines.indexOfFirst { "טקסט שנוקה" in it } + val encodedShift = requireNotNull(built.cleanShifts[lineIndex]) + assertTrue(lineWasModifiedByCleaning(encodedShift)) + assertEquals(4, generatedPrefixLength(encodedShift)) + val payload = BookPayload( + heTitle = "ספר בדיקה", enTitle = "Test Book", categoriesHe = listOf("תנך"), + lines = built.lines, refEntries = built.refs, headings = built.headings, + authors = emptyList(), description = null, heShortDesc = null, + pubDates = emptyList(), altStructures = emptyList(), + cleanShiftByLineIndex = built.cleanShifts, + ).precomputeLineData() + return requireNotNull(payload.precomputed).lineKeyHashes!![lineIndex] + } + + // The internal
forces cleanSefariaLine to modify this segment. + // Inserting a segment before it changes its generated prefix (א -> ב), + // but must not change the content key. + val before = cleanedLineHash(listOf("טקסט
שנוקה", "שורה שנייה")) + val after = cleanedLineHash(listOf("שורה חדשה", "טקסט
שנוקה", "שורה שנייה")) + assertContentEquals(IdAllocatorBindings.lineNaturalKeyHash("טקסט שנוקה"), before) + assertContentEquals(before, after, "cleaning must not make the generated prefix part of the key") + } + + @Test + fun `a legacy prefixed line without an heRef still migrates`() { + // Build 1: pre-#1211 keys — no heRef, so the rendered (prefixed) content. + val build1 = InMemoryIdAllocator.load(null) + val bookId = build1.bookId("Sefaria", "ספר בדיקה") + val old = chapter(listOf("פסוק אלף", "פסוק בית"), withRefs = false) + val legacyOcc = LineOccurrenceCounter() + val legacyIds = old.lines.indices.map { idx -> + val h = requireNotNull(old.precomputed).legacyLineKeyHashes!![idx] + build1.lineId(bookId, h, legacyOcc.next(bookId, h)) + } + + // Build 2 keys on the raw segment; the shim must carry both ids over. + val statePath = Files.createTempDirectory("legacy-prefix").resolve("build_state.db") + build1.snapshotTo(statePath) + val build2 = InMemoryIdAllocator.load(statePath) + val newIds = allocate(build2, build2.bookId("Sefaria", "ספר בדיקה"), chapter(listOf("פסוק אלף", "פסוק בית"), withRefs = false)) + assertEquals(legacyIds, newIds) + assertEquals(2, build2.legacyLineKeysMigrated()) + assertEquals(0, build2.legacyLineKeysMissed()) + } +} diff --git a/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/LinePrecomputeTest.kt b/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/LinePrecomputeTest.kt index ba37a6a0..e2c046ee 100644 --- a/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/LinePrecomputeTest.kt +++ b/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/LinePrecomputeTest.kt @@ -2,6 +2,7 @@ package io.github.kdroidfilter.seforimlibrary.sefariasqlite import io.github.kdroidfilter.seforimlibrary.common.countVisibleChars import io.github.kdroidfilter.seforimlibrary.common.ids.IdAllocatorBindings +import io.github.kdroidfilter.seforimlibrary.common.ids.LegacyLineKey import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll @@ -67,10 +68,15 @@ class LinePrecomputeTest { p.lines.forEachIndexed { idx, content -> val refEntry = pre.refsByLineIndex[idx] assertContentEquals( - IdAllocatorBindings.lineNaturalKeyHash(content, refEntry?.heRef), + IdAllocatorBindings.lineNaturalKeyHash(content), hashes[idx], "hash mismatch at line $idx", ) + assertContentEquals( + LegacyLineKey.hash(content, refEntry?.heRef), + requireNotNull(pre.legacyLineKeyHashes)[idx], + "legacy hash mismatch at line $idx", + ) assertEquals(20, hashes[idx].size, "natural key hash must stay 20 bytes at line $idx") assertEquals(countVisibleChars(content), charCounts[idx], "charCount mismatch at line $idx") assertEquals( @@ -91,13 +97,18 @@ class LinePrecomputeTest { } @Test - fun heRefIsPreferredOverContentForLinesThatHaveOne() { + fun heRefNeverEntersTheNaturalKey() { val p = payload().precomputeLineData() - val hashes = requireNotNull(p.precomputed).lineKeyHashes!! - // Line 1 has a heRef → keyed on the ref, not the content. - assertContentEquals(IdAllocatorBindings.lineNaturalKeyHash("anything", "בראשית א׳:א׳"), hashes[1]) - // Line 0 has none → keyed on content. - assertContentEquals(IdAllocatorBindings.lineNaturalKeyHash(p.lines[0], null), hashes[0]) + val pre = requireNotNull(p.precomputed) + val hashes = pre.lineKeyHashes!! + // Line 1 has a heRef; line 0 has none — both keyed on content alone. + assertContentEquals(IdAllocatorBindings.lineNaturalKeyHash(p.lines[1]), hashes[1]) + assertContentEquals(IdAllocatorBindings.lineNaturalKeyHash(p.lines[0]), hashes[0]) + // The legacy array still carries the heRef — it only seeds the shim. + assertContentEquals( + LegacyLineKey.hash("anything", "בראשית א׳:א׳"), + pre.legacyLineKeyHashes!![1], + ) } @Test @@ -106,6 +117,7 @@ class LinePrecomputeTest { val pre = requireNotNull(p.precomputed) assertEquals(0, pre.lineCount) assertEquals(0, pre.lineKeyHashes!!.size) + assertEquals(0, pre.legacyLineKeyHashes!!.size) assertEquals(0, pre.lineCharCounts!!.size) assertEquals(0, pre.lineIsHeading!!.size) } @@ -120,6 +132,7 @@ class LinePrecomputeTest { pre.release() assertNull(pre.lineKeyHashes) + assertNull(pre.legacyLineKeyHashes) assertNull(pre.lineCharCounts) assertNull(pre.lineIsHeading) // The inline-anchor pass holds refsByLineIndex for the whole build, and @@ -171,6 +184,9 @@ class LinePrecomputeTest { serial.lineKeyHashes!!.forEachIndexed { idx, expected -> assertContentEquals(expected, pre.lineKeyHashes!![idx], "hash mismatch at line $idx") } + serial.legacyLineKeyHashes!!.forEachIndexed { idx, expected -> + assertContentEquals(expected, pre.legacyLineKeyHashes!![idx], "legacy hash mismatch at line $idx") + } } } } diff --git a/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchorsTest.kt b/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchorsTest.kt index 2e2058ac..f431211d 100644 --- a/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchorsTest.kt +++ b/generator/sefariasqlite/src/jvmTest/kotlin/io/github/kdroidfilter/seforimlibrary/sefariasqlite/SefariaCharLevelAnchorsTest.kt @@ -17,6 +17,14 @@ import kotlin.test.assertTrue class SefariaCharLevelAnchorsTest { + @Test + fun cleanedShiftKeepsPrefixLengthButRemainsInexactForAnchors() { + val encoded = cleanedLineShift(4) + assertTrue(lineWasModifiedByCleaning(encoded)) + assertEquals(4, generatedPrefixLength(encoded)) + assertEquals(0, generatedPrefixLength(CLEAN_MODIFIED)) + } + @Test fun parsesCharBasedCell() { val cell = parseCharLevelCell(