From 3c3e23890258e3f0a24004daa64b478f6febb31f Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Tue, 4 Aug 2026 21:16:42 -0700 Subject: [PATCH 1/8] Keep a paginated copy of a book per layout config B6 made a settings change replay its captured content instead of re-reading the EPUB, and measured the result on X3: 24.7 s for 736 pages, 27.1 s for 1240. That is 2.4-2.6x better than the 64 s cold build, and still 24-27 s of staring at a progress screen -- because nearly all of what is left is downstream of the capture point, in the wrap and the section writes, which no zip/inflate/XML skip can touch. The waste is that the work is thrown away. A book cached one set of files, so every wrap-relevant change overwrote the last one's pagination, and flipping back re-paid the whole replay. With portrait now a first-class flow and the default, the orientation toggle pays it too: the page box is wrap-relevant, so portrait<->landscape rebuilds all sections and overwrites the other orientation's. So name the files for the config that produced them, and keep more than one. Two hex digits of the wrap-relevant bits of `reader_layout_config`: size, weight, family, and the portrait page box. Sections become `SECTIONS/S.BIN` and the book index becomes `BK.BIN`, both inside FAT's 8.3 budget (`CACHE_SECTION_FILE_BYTES` 8 -> 10). The roadmap asked for a page-box bit to be *added* to the config, on the grounds that `reader_layout_config` predates portrait. It does not anymore -- READER_LAYOUT_VERSION v18 put one in bit 7 -- so the key only had to include it. Two axes stay out of the name on purpose. Spacing, because a spacing change re-walks heights over the same wrap points, so both spacings share one set of files exactly as they did before. And the wrap version plus panel salt, because a bump there has to retire *every* config: leaving them out means a bump rejects each index in its own header check and rebuilds in place, rather than stranding a fresh set of filenames per bump with no one left to delete the old. No signature took a config argument. Every per-config reader and writer derives the key from the `ReaderStore` it already holds, so no caller can name a file for one config and fill it with another's pagination. The three name parsers read bytes rather than slicing `&str`. A FAT short name is raw bytes and embedded-sdmmc renders them ISO-8859-1 (`c as char`), so a directory entry carrying a byte >= 0x80 arrives as a multi-byte char; slicing at the fixed field offsets would land inside one, and on the device a panic is a reset. A foreign or corrupt name in a cache directory is not a reason to reboot. A 10-byte file per book listing the resident configs, most recently used first. It does two jobs. Eviction: section files multiply per config, so a book holds at most CACHE_CONFIG_SLOTS (2) of them -- the flip and the flip back, which is the flow that hurts -- and a third arriving deletes the least recently used one's index and section files, nothing else. Ordering by *use* rather than insertion is what keeps the config a reader has just returned to off the block. And it tells the readers that want a book's config-*independent* facts which index file is there: source identity for the orphan sweep and the clear, the title for the Library list, the TOC and labels a replay has to carry into the index it rewrites. Those three used to open one fixed BOOK.BIN. `read_cache_header` is the one that had to be careful, because its `Absent` is what licenses the clear path to delete a cache directory -- against a 28-bit key whose collisions the format admits. It works the registry's slots, then falls back to a bounded directory listing, so a lost registry cannot make a cache that is plainly there read as deletable; a registry present but undecodable, an index that opens and will not parse, *or a listing that would not run* all read as `Unreadable` instead. That last one is why the listing returns an option rather than an empty vector: a card that refused to enumerate has not said the directory is empty, and reading it that way would hand the clear a delete it had not earned. `Absent` is reached only when nothing is there at all. `read_cached_book_title` skips the listing deliberately -- it runs once per book at catalog scan, and a miss there costs a label, not a cache. The other readers of config-independent facts share that miss semantics; `read_cache_header` is the only one where a failed listing has to be told from an empty directory. `adopt_layout_config` called `delete_layout_config_artifacts_in` and then wrote the promoted registry regardless: the helper returned nothing, and both the index delete and the section sweep discarded their failures. A card that refused a delete -- an I/O fault, or no space to write back the directory entry -- got a registry that said the eviction happened. That unregisters an intact cache. The surviving files still load if the reader flips back to that config (the per-config load names `BK.BIN` directly rather than through the registry), but nothing counts them anymore: the next eviction takes one of the two the registry does name and walks past a full section set that no later pass will ever look at again. Repeat it under intermittent failures and the two-config storage bound is gone. It is worst exactly when the delete was needed for space -- the build that follows can then fail for want of it, on a card whose older, intact cache the registry has just hidden. So the deletion path answers the question the registry write depends on. `delete_layout_config_artifacts_in` returns whether every one of that config's files is gone -- deleted here or already absent -- and reads false on anything it cannot account for: a refused delete, a `SECTIONS/` that would not open, a sweep that did not finish. `sweep_section_files` returns the same kind of answer, true only when a listing came back with nothing left matching `wanted`. The index still goes first, so an interrupted eviction leaves an unreadable set (stray sections, no index) rather than an index promising sections that are gone. On failure the adoption is abandoned and the stored registry left untouched, so it still names the config whose files are still there. That config stays counted, and the next open under this one retries the same deletes. `LayoutConfigAdoption::eviction_failed` names it; `evicted` is set only after the deletion actually finished. The alternative -- failing the open -- was not taken. Eviction only runs when the config is not resident, which is exactly when a build was going to happen anyway, so aborting would mean refusing to open the book because a delete failed. The build proceeds and writes under a config the registry does not name; that index is still found by the next open under the same config, and by the unlisted-index scan, and it becomes registered as soon as an eviction succeeds. What can no longer happen is a registry claiming an eviction that did not occur. The registry write itself can also refuse, and that one is not fatal either -- for the same reason -- but it does need repairing rather than shrugging off. `CFG.BIN` is opened create-or-truncate, so a refused write leaves not the previous registry but nothing decodable. Reading that as an empty registry is what would lose the bound: every config on the card stops being counted at once, so no eviction ever takes them and each later open is free to stack another full section set on top. One failed small write would start an accumulation that only a manual clear ends. So a `CFG.BIN` that is there and will not decode is rebuilt from the index files the directory actually holds, using the same bounded listing `read_cache_header` falls back to. Ordering cannot be recovered -- a listing does not say which config was read last -- but the count can, and the count is what eviction needs. A card already over the bound leaves the extras unnamed for one more open; the eviction that follows brings the number down, and later opens converge. A registry that is not there *at all* still starts empty, so a book being opened for the first time does not pay for a listing. `LayoutConfigAdoption::registry_write_failed` reports the refusal so the log names a card that is failing small writes. Rebasing onto #59 put a second deleter in the same directory. That change prunes the section files a completed rebuild stranded: sections are numbered by a dense `0..count` ordinal, so a rebuild deriving fewer of them leaves the tail past its new count referenced by nothing, and the publish tail deletes it once the index has landed. Per-config naming breaks the assumption underneath it. It matched `S###.BIN` and ranged over the ordinal alone, which was sound when a book had one paginated copy. Now there are two, each numbering its sections from zero, so their ordinal ranges overlap completely -- and the config whose sections sit past the publishing config's count is not stranded at all. Left as it was, the prune deletes a working paginated copy of the book, which is the one thing this whole change exists to keep. So the prune matches on both halves of the name. `section_file_name_parts` returns the key and the ordinal from one parser, `layout_key_of_section_file_name` is now expressed through it, and `prune_orphan_sections` takes the key from the same `ReaderStore` whose pagination it just published rather than from an argument -- the convention above, for the same reason. Two smaller consequences. The keyed name is ten bytes where the old one was eight, so `S###.BIN` no longer parses as a section at all: the prune leaves the pre-per-config files alone, which is right, because retiring those is the legacy purge's job and it is gated on `BOOK.BIN`. And the prune would otherwise have gone quietly dead rather than wrong -- nothing on the card would have matched its pattern any more, so #59's insurance would have been silently spent. The roadmap suggested bumping CACHE_V2_VERSION since the index name changed. It is not needed and it would cost something: nothing reads the old names, so a bump would only invalidate CONT.BIN too and turn the one-time transition from a replay into a full EPUB re-parse. Instead the first open that finds an unkeyed BOOK.BIN deletes it and the unkeyed `S.BIN` files. The check is one failed file open on every later open, since nothing writes that name again. Which makes `BOOK.BIN` the retry marker, and it is deleted last, once the section sweep reports itself finished. Taking it first would strand whatever the sweep could not: nothing reads the unkeyed names, no registry counts them, and nothing else would ever bring an open back to look. A marker left behind costs one failed file open per later open and buys the retry; nothing reads `BOOK.BIN` itself anymore, so a marker outliving its sections misleads no reader. The purge still returns only whether there was anything to purge -- no caller needs the stronger answer -- but it no longer discards the sweep result that decides when the marker may go. TOC.BIN, COVER.BIN, CONT.BIN and the reading position do not depend on the layout config and are untouched by eviction and by the purge. Flash +7,082 B text on X3 (6,704,500 -> 6,711,582): `.text` +6,802 and `.rodata` +280. X4 costs the same to within 2 bytes: +7,084 B (6,689,378 -> 6,696,462), `.text` +6,804 and `.rodata` +280. No new static RAM on either device. `data` (6,644 both) and `bss` (326,877 on X3, 322,645 on X4) are byte-identical to the baseline, as is `.dram2_prev_fb` (52,273 on X3, 48,001 on X4). Measured for the whole branch against its merge base with `llvm-size`, one device at a time in this same worktree -- both traps that manufacture fake deltas here, an X3 binary left at the shared output path and a baseline built in a different directory shifting `.rodata`, avoided. On the card, one more paginated copy of the book -- the section files are the bulk, and the 2-slot cap is what bounds it. Stack: the deepest frames are unchanged. `adopt_layout_config` peaks around 400 B, dominated by a 16x12-byte name-staging vector modelled on `empty_sections_dir`'s, and it runs and returns before the EPUB chain's 13,840 B frame rather than nesting inside it. proto: key derivation (the orientation flip must land on a different key, a version bump must not), name recognition against both schemes and near-misses, registry ordering and eviction, encode round-trip and the decodes that must be rejected -- out-of-range key, repeated key, short buffer. Both halves of a section name round-trip for every key and ordinal the prune ranges over, and the legacy eight-character form is rejected outright. Plus three crafted non-ASCII names, each the exact byte length its parser accepts and each putting a char boundary inside a field offset that parser reads: the case the byte-wise rewrite above exists for. Against the old slicing all three panic. reader-cache, on the existing FAT16 fault-injection harness: a second config does not overwrite the first and the flip back loads without a rebuild; a third config evicts the least recently used one and leaves the survivor and COVER.BIN alone; re-reading a resident config moves it off the eviction block; the legacy purge takes both old artifacts and spares the settings-independent ones; a cache whose registry was deleted still names its book; a registry that is there and unusable fails closed as `Unreadable`; and a clear with two configs resident reports success, which it can only do by reclaiming every per-config file, since it verifies by listing the directory. Two more for the eviction reporting, one for a blocked index delete and one for a blocked section delete after the index went. They block a named file by holding it open, which this embedded-sdmmc rev refuses to reopen -- the same `RemoveStatus::Failed` an I/O fault produces, aimed at one file instead of the Nth write. Each asserts that nothing is reported evicted, that the failure is named, that the files survive, and that a retry once the block clears evicts the same key, which is what proves the registry still named it. And four for the failure paths above. A refused registry write, blocked the same way, is reported rather than swallowed. A registry truncated exactly as a refused write leaves it still evicts on the next open, which an empty reading could not do. A legacy sweep blocked mid-pass keeps `BOOK.BIN`, and the retry once the block clears takes both the section and the marker. And a property test: `read_cache_header` is run once per read the pass makes, with that read failed, and none of them may answer `Absent` for a cache that is on the card -- `Unreadable` is fine, since it fails closed. Each of the three behavioural ones fails against the code before it: the sweep test on the stranded marker, the registry test on an eviction that never happens, and the property test on a concrete fault position that produced `Absent`. And one for the prune, which #59's own four do not cover because before this change there was only ever one config: a shrinking rebuild under the second config must take its own stranded tail and leave every one of the first config's sections. It fails against an ordinal-only prune on the first section it finds missing. #59's tests came across keyed -- its helper now names section files through the config the store is holding, so a drifting key cannot leave them passing against the wrong file -- and its stray-name test grew an unkeyed `S003.BIN`, whose ordinal is inside the pruned range, to pin that the prune leaves the legacy scheme to the purge. Verified with `tools/check.sh fast`, `tools/check.sh firmware` (X4 and X3) and `tools/check.sh fmt`, all green, plus clippy on `proto` and `reader-cache` with their tests. The emulator leg was not re-run: nothing here touches layout, rendering, typography or the golden frames. `publish.rs`'s provisional-publish failure path still calls `empty_cache_dir`, so a torn index write for one config now takes the other config's good cache with it. Scoping that to the failing config is the right fix; it changes a fault path with tests pinning it, and the current behaviour degrades to a rebuild rather than to anything incorrect, so it is left out of this diff. Two books whose 28-bit keys collide can now hold indexes under different configs in the same directory, where the single-file scheme could only let one overwrite the other. `read_cache_header` answers with the first index that reads, so it may report the wrong book's header, and a clear that does match deletes the colliding book's files along with its own. The identity stamp still stops the clear from deleting against a header it cannot match, and the collision was already destructive before this change, so this is a pre-existing limitation of the key width rather than a regression. Fixing it properly means passing source identity into adoption and purging derived files whose index belongs to another source, which is its own change. --- docs/ARCHITECTURE.md | 56 +- fw/src/book_build.rs | 19 + proto/src/cache.rs | 459 ++++++++++- reader-cache/src/files.rs | 1061 +++++++++++++++++++++----- reader-cache/src/publish.rs | 5 + reader-cache/tests/publish_faults.rs | 907 +++++++++++++++++++++- 6 files changed, 2268 insertions(+), 239 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d969ce59..5b4781a5 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -416,6 +416,7 @@ only window crossings re-read it. The currently open book sits in a separate scrolled. On boot/refresh, firmware first loads a window from the cached snapshot, then refreshes `/BOOKS` and card-root discovery in a storage command, streaming the fresh catalog out in batches without ever holding it +<<<<<<< HEAD whole. Discovery skips dot-prefixed entries, so the AppleDouble sidecar (`._.epub`) Finder writes beside every file it copies to a FAT card is not catalogued as a phantom, unopenable duplicate. The scan reads long @@ -423,7 +424,7 @@ filenames through a buffer sized to the FAT maximum, because an entry whose long name does not fit is presented under its short name, and the sidecar's short name (`_BOOK~1.EPU`) no longer carries the dot that identifies it. Entries are labeled with the book's real title from its cached -`BOOK.BIN`, falling back to the stored original-filename label for uploaded +book index, falling back to the stored original-filename label for uploaded 8.3-named books, then to the prettified file stem. Each fresh catalog write also sweeps `CACHE2` and reclaims caches whose stored source identity no longer matches any catalogued book, deleting the data files and the emptied @@ -433,24 +434,57 @@ cache/scan, and “No books found” only after a completed scan proves the card has no EPUBs. ```text -/XTEINK/CACHE2/E/BOOK.BIN +/XTEINK/CACHE2/E/CFG.BIN +/XTEINK/CACHE2/E/BK.BIN /XTEINK/CACHE2/E/TOC.BIN /XTEINK/CACHE2/E/COVER.BIN /XTEINK/CACHE2/E/CONT.BIN -/XTEINK/CACHE2/E/SECTIONS/S000.BIN -/XTEINK/CACHE2/E/SECTIONS/S001.BIN +/XTEINK/CACHE2/E/SECTIONS/S000.BIN +/XTEINK/CACHE2/E/SECTIONS/S001.BIN /XTEINK/CATALOG.BIN /XTEINK/LABELS/.TXT /XTEINK/STATEA.BIN /XTEINK/STATEB.BIN ``` -`BOOK.BIN` holds a `BookV2Header`, one `BookV2SectionRecord` per section (spine, -start page, page count, partial), TOC records, and a string blob for title, -author, and TOC titles. Section files hold a `SectionV2Header`, page records, -block records, per-block paragraph flags, and the UTF-8 text blob of that -section's pre-wrapped lines. `TOC.BIN` is a per-book chapter-list sidecar for -the Chapters overview, distinct from the TOC records inside `BOOK.BIN`. +The book index and the section files are per *layout config* — `` is two +hex digits of the wrap-relevant bits of `reader_layout_config` (type size, +weight, family, and the portrait page box), so a book keeps a separate +paginated copy per setting it has been read under and flipping back to one is +a cache hit rather than a full re-wrap. `CFG.BIN` lists the resident configs, +most recently used first; a book holds at most `CACHE_CONFIG_SLOTS` (2) of +them and the least recently used one's files are deleted when a third +arrives. The registry is written only once that deletion has finished: an +open that cannot remove every file of the config it is evicting leaves the +registry naming it, so it stays counted and the next open retries, rather +than unregistering a set of files nothing would ever look at again. A +`CFG.BIN` that is there and will not decode — which is what a refused write +leaves behind, the file being opened create-or-truncate — is rebuilt from the +index files the directory actually holds rather than read as an empty +registry: reading it empty would stop every config on the card from being +counted, so no eviction would ever take them and each later open could add +another full set. It is +also how the readers that want a book's config-*independent* +facts — source identity for the orphan sweep, the title for the Library list, +the TOC for a replay — know which index file is there. The wrap-rule version +and panel salt deliberately stay out of the name: a bump there has to retire +every config, which it does by being rejected in each index's own header. +A completed rebuild that derives fewer sections than the one before it prunes +the tail it stranded, after its index is on the card and only within its own +config: both resident configs number their sections from zero, so a prune going +by ordinal alone would delete the other config's pagination. +A pre-per-config cache (an unkeyed `BOOK.BIN` and `S.BIN` files) is +deleted on the first open that finds it, `BOOK.BIN` last: its presence is the +only thing that sends a later open back through the purge, so taking it before +the section sweep has finished would strand whatever the sweep could not. The +prune leaves those unkeyed names alone — retiring them is the purge's job. + +`BK.BIN` holds a `BookV2Header`, one `BookV2SectionRecord` per section +(spine, start page, page count, partial), TOC records, and a string blob for +title, author, and TOC titles. Section files hold a `SectionV2Header`, page +records, block records, per-block paragraph flags, and the UTF-8 text blob of +that section's pre-wrapped lines. `TOC.BIN` is a per-book chapter-list sidecar +for the Chapters overview, distinct from the TOC records inside the index. `CONT.BIN` records the build's `push_block` stream — the settings-independent half of the work — so a type-settings or orientation change replays it into the same sink instead of re-reading and re-parsing the EPUB. It is purely an @@ -459,7 +493,7 @@ captured, and any read or decode failure deletes it and falls back to the EPUB. A cold build does not run to the end before the reader sees the book. It publishes as soon as the section holding the requested page is written, marking -`BOOK.BIN` partial, and finishes the spine in slices from an idle branch of the +the index partial, and finishes the spine in slices from an idle branch of the display task's loop — so the first page arrives in about a second rather than after the whole walk, and every other task keeps getting scheduled meanwhile. Only the pages built so far are addressable until the walk finishes, and the diff --git a/fw/src/book_build.rs b/fw/src/book_build.rs index 8d65454f..688afca8 100644 --- a/fw/src/book_build.rs +++ b/fw/src/book_build.rs @@ -607,6 +607,25 @@ where let cache_key = proto::cache::cache_key_for(display_name.as_str(), source_identity.1); library.set_cache_key(cache_key.as_str()); esp_println::println!("epub: stage ResolveCatalogEntry key={}", cache_key.as_str()); + // Claim this open's layout config as the book's most recently used before + // anything reads or writes under it: the per-config index and section + // files are named for it, so the registry has to know it, and the least + // recently used config's files have to be gone before a build adds a + // third set. + // + // An eviction that would not go through leaves this config unadopted: the + // build below still runs and writes under it, but the registry keeps + // naming the config whose files are still on the card, so the next open + // retries the eviction rather than losing track of a full section set. + let adoption = files::adopt_layout_config(root, cache_key.as_str(), library); + esp_println::println!( + "epub: layout config resident={} evicted={:?} evict_failed={:?} legacy_purged={} registry_write_failed={}", + adoption.resident, + adoption.evicted, + adoption.eviction_failed, + adoption.purged_legacy, + adoption.registry_write_failed + ); esp_println::println!( "epub: stage TryV2BookIndexFast page={}", target_pages as u32 diff --git a/proto/src/cache.rs b/proto/src/cache.rs index bff48e97..696febda 100644 --- a/proto/src/cache.rs +++ b/proto/src/cache.rs @@ -40,7 +40,47 @@ pub const CACHE_BOOK_FILE: &str = "BOOK.BIN"; pub const CACHE_COVER_FILE: &str = "COVER.BIN"; pub const CACHE_STATE_FILE: &str = "STATE.BIN"; pub const CACHE_KEY_BYTES: usize = 8; -pub const CACHE_SECTION_FILE_BYTES: usize = 8; +pub const CACHE_SECTION_FILE_BYTES: usize = 10; +/// Width of the pre-per-config section name (`S.BIN`). Kept so the +/// upgrade purge can recognize what an older firmware left behind. +pub const LEGACY_SECTION_FILE_BYTES: usize = 8; +pub const BOOK_INDEX_FILE_PREFIX: &str = "BK"; +pub const BOOK_INDEX_FILE_BYTES: usize = 8; + +/// The wrap-relevant bits of `ui::reading::reader_layout_config`: size (bits +/// 2-3), weight (bit 4), family (bits 5-6), and the portrait page box (bit +/// 7). Spacing (bits 0-1) only re-walks heights over the same wrap points, +/// and the wrap-version plus panel salt above bit 7 are global — a bump +/// there has to retire every config, not earn each one its own file. So +/// neither belongs in a per-config file name. +pub const LAYOUT_WRAP_CONFIG_MASK: u16 = 0b1111_1100; +/// How many distinct layout keys the mask can produce, and so the exclusive +/// upper bound on a valid key. +pub const LAYOUT_KEY_VALUES: usize = 64; + +/// The per-config cache key packed out of a layout config: the six +/// wrap-relevant bits, right-aligned, so it renders as two hex digits. +pub fn layout_cache_key(font_config: u16) -> u8 { + ((font_config & LAYOUT_WRAP_CONFIG_MASK) >> 2) as u8 +} + +/// Resident-config registry (`CFG.BIN`): which layout configs a book keeps +/// paginated on the card, most recently used first. +/// +/// Two things need it. Eviction: section files multiply per config, so a +/// book keeps at most [`CACHE_CONFIG_SLOTS`] of them and the least recently +/// used one's files are deleted when a third arrives. And the readers that +/// want a book's *config-independent* facts — its source identity, its +/// title, its TOC — which live inside a per-config index and so need to be +/// told which index is there. +pub const CACHE_CONFIG_FILE: &str = "CFG.BIN"; +pub const CACHE_CONFIG_MAGIC: u32 = 0x5834_4746; // X4GF +pub const CACHE_CONFIG_VERSION: u16 = 1; +/// How many paginated copies of one book the card keeps. Two is what the +/// flows that hurt need: a size or orientation flip and the flip back. +pub const CACHE_CONFIG_SLOTS: usize = 2; +pub const CACHE_CONFIG_HEADER_BYTES: usize = 8; +pub const CACHE_CONFIG_BYTES: usize = CACHE_CONFIG_HEADER_BYTES + CACHE_CONFIG_SLOTS; pub const BOOK_HEADER_BYTES: usize = 16; pub const SPINE_RECORD_BYTES: usize = 12; pub const TOC_RECORD_BYTES: usize = 24; @@ -617,13 +657,220 @@ pub fn cache_key_for(source_path: &str, source_len: u32) -> String(spine: u16, out: &mut String) { +/// Section file name for one layout config: `S.BIN`, where +/// `key` is the two hex digits of [`layout_cache_key`]. Six base characters +/// plus the extension keeps the name inside FAT's 8.3 budget. +/// +/// Naming the config is what lets a book keep more than one paginated copy +/// on the card, so flipping back to a previously-built type size or +/// orientation is a cache hit instead of a full re-wrap. +pub fn section_file_name(layout_key: u8, spine: u16, out: &mut String) { out.clear(); let _ = out.push('S'); + push_hex(out, layout_key as u32, 2); push_dec3(out, spine); let _ = out.push_str(".BIN"); } +/// Book index file name for one layout config: `BK.BIN`. The +/// pre-per-config [`CACHE_BOOK_FILE`] name is never written anymore; it +/// survives only so the upgrade path can recognize and delete it. +pub fn book_index_file_name(layout_key: u8, out: &mut String) { + out.clear(); + let _ = out.push_str(BOOK_INDEX_FILE_PREFIX); + push_hex(out, layout_key as u32, 2); + let _ = out.push_str(".BIN"); +} + +/// The layout config a per-config book index name carries, or `None` when +/// `name` is not one — so a directory listing can find a book's indexes +/// without the registry naming them. +pub fn layout_key_of_book_index_file_name(name: &str) -> Option { + // Read as bytes, not sliced as a `str`. A FAT short name is raw bytes, and + // embedded-sdmmc renders them ISO-8859-1, so a directory entry carrying a + // byte >= 0x80 arrives here as a multi-byte char. Slicing at these fixed + // offsets would then land inside one and panic, which on the device is a + // reset. The field offsets are all ASCII when the name is really ours, so + // any name they cannot be read out of simply is not one. + let bytes = name.as_bytes(); + if bytes.len() != BOOK_INDEX_FILE_BYTES + || !bytes[..2].eq_ignore_ascii_case(BOOK_INDEX_FILE_PREFIX.as_bytes()) + || !bytes[4..].eq_ignore_ascii_case(b".BIN") + { + return None; + } + parse_hex2(&bytes[2..4]).filter(|key| (*key as usize) < LAYOUT_KEY_VALUES) +} + +/// The layout config a per-config artifact name carries, or `None` when the +/// name belongs to neither scheme. Used by eviction (which deletes one +/// config's section files) and by the legacy purge (which deletes the +/// unkeyed `S.BIN` files an older firmware wrote). +pub fn layout_key_of_section_file_name(name: &str) -> Option { + section_file_name_parts(name).map(|(layout_key, _)| layout_key) +} + +/// The layout config *and* section ordinal a per-config section name carries. +/// +/// The ordinal is the dense `0..count` counter the walk assigns as it flushes, +/// which is what the orphan prune ranges over: a rebuild producing fewer +/// sections leaves the tail past its new count on the card. The prune needs +/// both halves — the ordinal to know what is stranded, and the key so it prunes +/// only the config it just published and leaves the other resident config's +/// pagination alone. +pub fn section_file_name_parts(name: &str) -> Option<(u8, u16)> { + // Bytes rather than `str` slices, for the reason above. + let bytes = name.as_bytes(); + if bytes.len() != CACHE_SECTION_FILE_BYTES + || !bytes[..1].eq_ignore_ascii_case(b"S") + || !bytes[6..].eq_ignore_ascii_case(b".BIN") + { + return None; + } + let key = parse_hex2(&bytes[1..3])?; + if (key as usize) >= LAYOUT_KEY_VALUES { + return None; + } + let mut ordinal = 0u16; + for &byte in &bytes[3..6] { + if !byte.is_ascii_digit() { + return None; + } + ordinal = ordinal * 10 + u16::from(byte - b'0'); + } + Some((key, ordinal)) +} + +/// Whether `name` is a section file from the single-config scheme that +/// preceded [`section_file_name`]'s layout key (`S.BIN`, eight +/// characters). Nothing writes these; the upgrade path deletes them. +pub fn is_legacy_section_file_name(name: &str) -> bool { + // Bytes rather than `str` slices, for the reason above. + let bytes = name.as_bytes(); + bytes.len() == LEGACY_SECTION_FILE_BYTES + && bytes[..1].eq_ignore_ascii_case(b"S") + && bytes[1..4].iter().all(|byte| byte.is_ascii_digit()) + && bytes[4..].eq_ignore_ascii_case(b".BIN") +} + +/// The resident layout configs for one book, most recently used first. +/// +/// Ordering is the whole point: slot 0 is the config the book was last read +/// under (so a reader after config-independent facts knows which index file +/// exists), and the last slot is what eviction takes when a new config +/// arrives. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Default)] +pub struct LayoutConfigRegistry { + keys: [u8; CACHE_CONFIG_SLOTS], + count: usize, +} + +impl LayoutConfigRegistry { + pub fn new() -> Self { + Self::default() + } + + /// The resident keys, most recently used first. + pub fn keys(&self) -> &[u8] { + &self.keys[..self.count] + } + + /// The config the book was last read under, if it has ever been built. + pub fn most_recent(&self) -> Option { + self.keys().first().copied() + } + + pub fn contains(&self, layout_key: u8) -> bool { + self.keys().contains(&layout_key) + } + + /// Move `layout_key` to the front, inserting it if it is new. Returns the + /// key pushed out of the last slot, whose files the caller must delete — + /// `None` when nothing was evicted, including when `layout_key` was + /// already resident. + pub fn promote(&mut self, layout_key: u8) -> Option { + if let Some(position) = self.keys().iter().position(|key| *key == layout_key) { + self.keys[..=position].rotate_right(1); + return None; + } + let evicted = if self.count == CACHE_CONFIG_SLOTS { + Some(self.keys[CACHE_CONFIG_SLOTS - 1]) + } else { + self.count += 1; + None + }; + self.keys[..self.count].rotate_right(1); + self.keys[0] = layout_key; + evicted + } + + /// Drop `layout_key` from the registry, for a caller that has just + /// deleted its files. + pub fn forget(&mut self, layout_key: u8) { + if let Some(position) = self.keys().iter().position(|key| *key == layout_key) { + self.keys[position..self.count].rotate_left(1); + self.count -= 1; + self.keys[self.count] = 0; + } + } +} + +pub fn encode_layout_config_registry( + registry: &LayoutConfigRegistry, + out: &mut [u8], +) -> Result { + require(out, CACHE_CONFIG_BYTES)?; + write_u32(out, 0, CACHE_CONFIG_MAGIC); + write_u16(out, 4, CACHE_CONFIG_VERSION); + write_u16(out, 6, registry.count as u16); + out[CACHE_CONFIG_HEADER_BYTES..CACHE_CONFIG_BYTES].copy_from_slice(®istry.keys); + Ok(CACHE_CONFIG_BYTES) +} + +pub fn decode_layout_config_registry(input: &[u8]) -> Result { + require(input, CACHE_CONFIG_BYTES)?; + if read_u32(input, 0)? != CACHE_CONFIG_MAGIC { + return Err(CacheError::BadMagic); + } + if read_u16(input, 4)? != CACHE_CONFIG_VERSION { + return Err(CacheError::BadVersion); + } + let count = read_u16(input, 6)? as usize; + if count > CACHE_CONFIG_SLOTS { + return Err(CacheError::BadLength); + } + let mut keys = [0u8; CACHE_CONFIG_SLOTS]; + keys[..count].copy_from_slice(&input[CACHE_CONFIG_HEADER_BYTES..][..count]); + // Slots past `count` are left zero rather than read: they hold whatever + // the last shorter registry wrote there, and carrying that through would + // make two registries with identical resident keys compare unequal. + // + // An out-of-range or repeated key would name a file that cannot exist or + // hand eviction a slot that is not really free, so a registry carrying + // one is rejected outright rather than repaired: the caller rebuilds it + // from the config it is opening. + for (index, key) in keys[..count].iter().enumerate() { + if (*key as usize) >= LAYOUT_KEY_VALUES || keys[..index].contains(key) { + return Err(CacheError::BadLength); + } + } + Ok(LayoutConfigRegistry { keys, count }) +} + +fn parse_hex2(bytes: &[u8]) -> Option { + let mut value = 0u8; + for &byte in bytes { + let digit = match byte { + b'0'..=b'9' => byte - b'0', + b'a'..=b'f' => byte - b'a' + 10, + b'A'..=b'F' => byte - b'A' + 10, + _ => return None, + }; + value = (value << 4) | digit; + } + Some(value) +} + pub fn encode_book_header(header: BookCacheHeader, out: &mut [u8]) -> Result { require(out, BOOK_HEADER_BYTES)?; write_u32(out, 0, CACHE_MAGIC); @@ -1841,10 +2088,210 @@ mod tests { ); let mut name = String::::new(); - section_file_name(7, &mut name); - assert_eq!(name.as_str(), "S007.BIN"); - section_file_name(1234, &mut name); - assert_eq!(name.as_str(), "S999.BIN"); + section_file_name(0, 7, &mut name); + assert_eq!(name.as_str(), "S00007.BIN"); + section_file_name(0x3F, 1234, &mut name); + assert_eq!(name.as_str(), "S3F999.BIN"); + + let mut index = String::::new(); + book_index_file_name(0x2A, &mut index); + assert_eq!(index.as_str(), "BK2A.BIN"); + } + + #[test] + fn layout_key_takes_the_wrap_relevant_bits_only() { + // version 18 << 8 | portrait | family=1 | weight=1 | size=2 | spacing=3 + let portrait = (18u16 << 8) | (1 << 7) | (1 << 5) | (1 << 4) | (2 << 2) | 3; + let landscape = portrait & !(1 << 7); + // Spacing is masked off: it re-walks heights over the same wrap + // points, so both spacings share one set of section files. + let relaxed = portrait & !0b11; + + assert_eq!(layout_cache_key(portrait), 0b10_1110); + assert_eq!(layout_cache_key(relaxed), layout_cache_key(portrait)); + // The orientation flip is the flow B7 exists for: it must land on a + // different key, or portrait and landscape overwrite each other. + assert_ne!(layout_cache_key(landscape), layout_cache_key(portrait)); + // The wrap version rides above the mask, so a bump retires every + // config's files in place instead of stranding a new name per bump. + assert_eq!( + layout_cache_key(portrait ^ (1 << 8)), + layout_cache_key(portrait) + ); + assert!((layout_cache_key(u16::MAX) as usize) < LAYOUT_KEY_VALUES); + } + + #[test] + fn per_config_artifact_names_are_recognized_and_keyed() { + let mut name = String::::new(); + for key in [0u8, 1, 0x0A, 0x3F] { + section_file_name(key, 42, &mut name); + assert_eq!(layout_key_of_section_file_name(name.as_str()), Some(key)); + assert!(!is_legacy_section_file_name(name.as_str())); + + let mut index = String::::new(); + book_index_file_name(key, &mut index); + assert_eq!( + layout_key_of_book_index_file_name(index.as_str()), + Some(key) + ); + } + + // The old scheme is recognized as legacy and never as keyed, so the + // purge and eviction passes cannot mistake one for the other. + assert!(is_legacy_section_file_name("S007.BIN")); + assert_eq!(layout_key_of_section_file_name("S007.BIN"), None); + assert_eq!(layout_key_of_book_index_file_name(CACHE_BOOK_FILE), None); + + // Neither scheme, and nothing that merely looks close. + assert_eq!(layout_key_of_section_file_name("SGG007.BIN"), None); + assert_eq!(layout_key_of_section_file_name("S4000A.BIN"), None); + assert_eq!(layout_key_of_section_file_name("S00007.TXT"), None); + assert_eq!(layout_key_of_section_file_name("POSA.BIN"), None); + assert!(!is_legacy_section_file_name("POSA.BIN")); + assert_eq!(layout_key_of_book_index_file_name("BK40.BIN"), None); + assert_eq!(layout_key_of_book_index_file_name("BKZZ.BIN"), None); + assert_eq!(layout_key_of_book_index_file_name("BK3F.TXT"), None); + } + + /// A directory entry these have to answer for without panicking. + /// + /// FAT stores short names as raw bytes and embedded-sdmmc renders them + /// ISO-8859-1 (`c as char`), so a name carrying a byte >= 0x80 reaches + /// these parsers as a multi-byte char. Each of the three below is the exact + /// byte length its parser accepts and puts a char boundary *inside* one of + /// the fixed offsets that parser reads, which is what a `str` slice cannot + /// survive. On the device a panic is a reset, and a foreign or corrupt name + /// in a cache directory is not a reason to reboot. + #[test] + fn a_non_ascii_name_is_rejected_rather_than_panicked_on() { + // 8 bytes: 'B','K','1', 2-byte char spanning offset 4, then ".BI"'s + // tail — the offset `layout_key_of_book_index_file_name` slices at. + let index_name = "BK1\u{e9}.BI"; + assert_eq!(index_name.len(), BOOK_INDEX_FILE_BYTES); + // 10 bytes, with a char spanning offset 6. + let section_name = "S1111\u{e9}.BI"; + assert_eq!(section_name.len(), CACHE_SECTION_FILE_BYTES); + // 8 bytes, with a char spanning offset 4. + let legacy_name = "S11\u{e9}.BI"; + assert_eq!(legacy_name.len(), LEGACY_SECTION_FILE_BYTES); + + for name in [index_name, section_name, legacy_name] { + assert_eq!(layout_key_of_book_index_file_name(name), None); + assert_eq!(layout_key_of_section_file_name(name), None); + assert_eq!(section_file_name_parts(name), None); + assert!(!is_legacy_section_file_name(name)); + } + } + + /// Both halves of a section name, which is what the orphan prune ranges + /// over: the ordinal says what a shrinking rebuild stranded, and the key + /// keeps the prune inside the config it just published. Two configs number + /// their sections from zero, so dropping the key would have one deleting the + /// other's pagination. + #[test] + fn a_section_name_yields_both_its_config_and_its_ordinal() { + let mut name = String::::new(); + for key in [0u8, 1, 0x0A, 0x3F] { + for ordinal in [0u16, 7, 42, 999] { + section_file_name(key, ordinal, &mut name); + assert_eq!(section_file_name_parts(name.as_str()), Some((key, ordinal))); + } + } + + // The legacy scheme carries an ordinal but no key, so it must not parse + // as a keyed name at all — the prune would otherwise take files the + // legacy purge owns. + assert_eq!(section_file_name_parts("S003.BIN"), None); + assert_eq!(section_file_name_parts("S12.BIN"), None); + assert_eq!(section_file_name_parts("NOTES.TXT"), None); + // A key past the 64 the config can encode, and a non-digit ordinal. + assert_eq!(section_file_name_parts("S40001.BIN"), None); + assert_eq!(section_file_name_parts("S3F0A1.BIN"), None); + } + + #[test] + fn layout_config_registry_orders_by_use_and_evicts_the_oldest() { + let mut registry = LayoutConfigRegistry::new(); + assert_eq!(registry.most_recent(), None); + + assert_eq!(registry.promote(4), None); + assert_eq!(registry.promote(9), None); + assert_eq!(registry.keys(), &[9, 4]); + assert_eq!(registry.most_recent(), Some(9)); + + // Re-using a resident config reorders without evicting: that is what + // makes the flip back to a previously-built config a cache hit. + assert_eq!(registry.promote(4), None); + assert_eq!(registry.keys(), &[4, 9]); + + // A third config takes the least recently used one's slot. + assert_eq!(registry.promote(17), Some(9)); + assert_eq!(registry.keys(), &[17, 4]); + + registry.forget(4); + assert_eq!(registry.keys(), &[17]); + registry.forget(4); + assert_eq!(registry.keys(), &[17]); + } + + #[test] + fn layout_config_registry_round_trips_and_rejects_nonsense() { + let mut registry = LayoutConfigRegistry::new(); + registry.promote(0x3F); + registry.promote(0); + let mut bytes = [0u8; CACHE_CONFIG_BYTES]; + assert_eq!( + encode_layout_config_registry(®istry, &mut bytes), + Ok(CACHE_CONFIG_BYTES) + ); + assert_eq!(decode_layout_config_registry(&bytes), Ok(registry)); + + // A shorter registry decodes to exactly its resident keys, so its + // unused slot cannot resurrect an already-evicted config. + let mut single = LayoutConfigRegistry::new(); + single.promote(5); + encode_layout_config_registry(&single, &mut bytes).expect("registry encodes"); + bytes[CACHE_CONFIG_BYTES - 1] = 0x2A; + assert_eq!(decode_layout_config_registry(&bytes), Ok(single)); + + encode_layout_config_registry(®istry, &mut bytes).expect("registry encodes"); + let mut corrupt = bytes; + corrupt[0] = b'?'; + assert_eq!( + decode_layout_config_registry(&corrupt), + Err(CacheError::BadMagic) + ); + corrupt = bytes; + corrupt[4] = CACHE_CONFIG_VERSION as u8 + 1; + assert_eq!( + decode_layout_config_registry(&corrupt), + Err(CacheError::BadVersion) + ); + corrupt = bytes; + corrupt[6] = CACHE_CONFIG_SLOTS as u8 + 1; + assert_eq!( + decode_layout_config_registry(&corrupt), + Err(CacheError::BadLength) + ); + // Out of range for the mask. + corrupt = bytes; + corrupt[CACHE_CONFIG_HEADER_BYTES] = LAYOUT_KEY_VALUES as u8; + assert_eq!( + decode_layout_config_registry(&corrupt), + Err(CacheError::BadLength) + ); + // A repeat would hand eviction a slot that is not really free. + corrupt = bytes; + corrupt[CACHE_CONFIG_HEADER_BYTES + 1] = corrupt[CACHE_CONFIG_HEADER_BYTES]; + assert_eq!( + decode_layout_config_registry(&corrupt), + Err(CacheError::BadLength) + ); + assert_eq!( + decode_layout_config_registry(&bytes[..CACHE_CONFIG_BYTES - 1]), + Err(CacheError::BufferTooSmall) + ); } #[test] diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index f4d57aaa..b136ff6d 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -7,17 +7,20 @@ use display::font::FontStyle; use embedded_sdmmc::{Directory, File, Mode, TimeSource}; use heapless::String; use proto::cache::{ - decode_block, decode_book_v2_header, decode_book_v2_section, decode_cover_header, decode_page, - decode_section_v2_header, decode_toc, decode_toc_chapter, decode_toc_file_header, encode_block, - encode_book_v2_header, encode_book_v2_section, encode_content_header, - encode_content_record_header, encode_page, encode_section_v2_header, encode_toc, - encode_toc_file_header, section_file_name, BookV2Header, BookV2SectionRecord, ContentHeader, - ContentRecordHeader, SectionV2Header, TocFileHeader, BLOCK_RECORD_BYTES, BOOK_V2_HEADER_BYTES, - BOOK_V2_SECTION_RECORD_BYTES, CACHE_BOOK_FILE, CACHE_CONTENT_FILE, CACHE_COVER_FILE, - CACHE_ROOT_DIR, CACHE_SECTIONS_DIR, CACHE_SECTION_FILE_BYTES, CACHE_STATE_FILE, CACHE_TOC_FILE, - CACHE_V2_DIR, CONTENT_HEADER_BYTES, CONTENT_RECORD_HEADER_BYTES, COVER_HEADER_BYTES, - PAGE_RECORD_BYTES, SECTION_V2_HEADER_BYTES, TOC_CHAPTER_RECORD_BYTES, TOC_FILE_HEADER_BYTES, - TOC_RECORD_BYTES, + book_index_file_name, decode_block, decode_book_v2_header, decode_book_v2_section, + decode_cover_header, decode_layout_config_registry, decode_page, decode_section_v2_header, + decode_toc, decode_toc_chapter, decode_toc_file_header, encode_block, encode_book_v2_header, + encode_book_v2_section, encode_content_header, encode_content_record_header, + encode_layout_config_registry, encode_page, encode_section_v2_header, encode_toc, + encode_toc_file_header, is_legacy_section_file_name, layout_cache_key, + layout_key_of_book_index_file_name, layout_key_of_section_file_name, section_file_name, + section_file_name_parts, BookV2Header, BookV2SectionRecord, ContentHeader, ContentRecordHeader, + LayoutConfigRegistry, SectionV2Header, TocFileHeader, BLOCK_RECORD_BYTES, + BOOK_INDEX_FILE_BYTES, BOOK_V2_HEADER_BYTES, BOOK_V2_SECTION_RECORD_BYTES, CACHE_BOOK_FILE, + CACHE_CONFIG_BYTES, CACHE_CONFIG_FILE, CACHE_CONTENT_FILE, CACHE_COVER_FILE, CACHE_ROOT_DIR, + CACHE_SECTIONS_DIR, CACHE_SECTION_FILE_BYTES, CACHE_STATE_FILE, CACHE_TOC_FILE, CACHE_V2_DIR, + CONTENT_HEADER_BYTES, CONTENT_RECORD_HEADER_BYTES, COVER_HEADER_BYTES, PAGE_RECORD_BYTES, + SECTION_V2_HEADER_BYTES, TOC_CHAPTER_RECORD_BYTES, TOC_FILE_HEADER_BYTES, TOC_RECORD_BYTES, }; use proto::font_pack::{ decode_font_pack_name, FontPackFaceRecord, FontPackHeader, FONT_PACK_DIR, @@ -600,6 +603,12 @@ where /// without loading any section records. Used at boot restore so the Home /// progress bar has a denominator before the book is opened. Returns 0 if the /// index is missing, stale, or for another book. +/// +/// Takes the most recently used config's index, which is the one the reader +/// will open under while the settings stand. Like the check below, it does +/// not insist on the current config: a count from another one is a better +/// denominator than none, and always was — the pre-per-config version read +/// whatever config the single index had been left in. pub fn read_v2_book_total_pages< D, T, @@ -616,21 +625,18 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { - with_v2_book_file(root, key, Mode::ReadOnly, |file| { + with_any_v2_book_file(root, key, false, |file| { let mut header_bytes = [0u8; BOOK_V2_HEADER_BYTES]; - if read_exact_file(file, &mut header_bytes).is_err() { - return 0; - } - let Ok(header) = decode_book_v2_header(&header_bytes) else { - return 0; - }; + read_exact_file(file, &mut header_bytes).ok()?; + let header = decode_book_v2_header(&header_bytes).ok()?; if header.source_hash != source_identity.0 || header.source_size != source_identity.1 || header.custom_font_identity != library.custom_font_identity() + || header.total_pages == 0 { - return 0; + return None; } - header.total_pages + Some(header.total_pages) }) .unwrap_or(0) } @@ -761,7 +767,7 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { - with_v2_book_file(root, key, Mode::ReadOnly, |file| { + with_v2_book_file(root, key, layout_key_for(library), Mode::ReadOnly, |file| { let mut header_bytes = [0u8; BOOK_V2_HEADER_BYTES]; if read_exact_file(file, &mut header_bytes).is_err() { return BookIndexLoadResult::Invalid; @@ -822,6 +828,12 @@ where /// with the title learned the last time the book was opened. Returns false /// (leaving `out` untouched) when there is no cache for the book, the cached /// identity doesn't match, or the cache holds no title. +/// +/// The title is settings-independent, so any of the book's per-config +/// indexes answers; this takes the most recently used one the registry +/// names. A book whose registry is missing falls through to the label +/// fallbacks rather than paying a directory listing per row — this runs once +/// per book at catalog scan. pub fn read_cached_book_title< D, T, @@ -838,20 +850,16 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { - with_v2_book_file(root, key, Mode::ReadOnly, |file| { + with_any_v2_book_file(root, key, false, |file| { let mut header_bytes = [0u8; BOOK_V2_HEADER_BYTES]; - if read_exact_file(file, &mut header_bytes).is_err() { - return false; - } - let Ok(header) = decode_book_v2_header(&header_bytes) else { - return false; - }; + read_exact_file(file, &mut header_bytes).ok()?; + let header = decode_book_v2_header(&header_bytes).ok()?; if header.source_hash != source_identity.0 || header.source_size != source_identity.1 || header.title_text_bytes == 0 || header.title_text_bytes as usize > 64 { - return false; + return None; } // The title text sits after the header, the section records, and the // TOC block (records + text) -- the same body order write_v2_book_index @@ -860,20 +868,14 @@ where + header.section_count as u32 * BOOK_V2_SECTION_RECORD_BYTES as u32 + header.toc_count as u32 * TOC_RECORD_BYTES as u32 + header.toc_text_bytes; - if file.seek_from_start(title_offset).is_err() { - return false; - } + file.seek_from_start(title_offset).ok()?; let title_len = header.title_text_bytes as usize; let mut title = [0u8; 64]; - if read_exact_file(file, &mut title[..title_len]).is_err() { - return false; - } - let Ok(title_str) = core::str::from_utf8(&title[..title_len]) else { - return false; - }; + read_exact_file(file, &mut title[..title_len]).ok()?; + let title_str = core::str::from_utf8(&title[..title_len]).ok()?; out.clear(); let _ = out.push_str(title_str); - true + Some(true) }) .unwrap_or(false) } @@ -899,6 +901,12 @@ pub enum CacheHeader { /// count. Used by the orphan sweep to decide whether a cache still belongs to /// a book on the card, and by the clear to prove a key names the book it was /// asked about. +/// +/// Source identity is settings-independent, so any of the book's per-config +/// indexes answers. The registry names them; a cache whose registry was +/// lost is then searched by listing, because this is the reader whose `Absent` +/// gets a cache deleted and it must not report one for indexes that are +/// sitting right there. pub fn read_cache_header< D, T, @@ -929,15 +937,95 @@ where let xteink = open!(root.open_dir(CACHE_ROOT_DIR)); let cache = open!(xteink.open_dir(CACHE_V2_DIR)); let book_dir = open!(cache.open_dir(key)); - let file = open!(book_dir.open_file_in_dir(CACHE_BOOK_FILE, Mode::ReadOnly)); - let mut header_bytes = [0u8; BOOK_V2_HEADER_BYTES]; - if read_exact_file(&file, &mut header_bytes).is_err() { + let stored = read_layout_registry_in(&book_dir); + let registry = stored.unwrap_or_default(); + let mut name = String::::new(); + // An index file that opened but did not decode. Held rather than + // returned, so a second, readable config still gets to answer `Present` + // — but never downgraded to `Absent`, which is what licenses a delete. + let mut unreadable = false; + for layout_key in registry.keys() { + book_index_file_name(*layout_key, &mut name); + match read_book_index_header(&book_dir, name.as_str()) { + Some(Some(header)) => return CacheHeader::Present(header), + Some(None) => unreadable = true, + None => {} + } + } + // The registry's slots are exhausted. Anything else in the directory + // carrying an index name gets its turn, so a lost registry cannot make a + // cache that is plainly there read as absent and get it deleted. + // + // A listing that would not run is the same hazard as an index that would + // not decode: it leaves the directory's owner unestablished. Falling + // through to `Absent` would hand the clear path a delete it has not earned, + // on a 28-bit key whose collisions the format admits. + let Some(unlisted_names) = book_index_names_unlisted_by(&book_dir, ®istry) else { + return CacheHeader::Unreadable; + }; + for unlisted in unlisted_names { + match read_book_index_header(&book_dir, unlisted.as_str()) { + Some(Some(header)) => return CacheHeader::Present(header), + Some(None) => unreadable = true, + None => {} + } + } + // Either an index we could not interpret, or a registry that is there + // and says nothing usable: both mean this directory holds something + // whose owner we cannot establish, which is exactly what `Unreadable` + // stops a delete for. + if unreadable || (stored.is_none() && registry_present(&book_dir)) { return CacheHeader::Unreadable; } - match decode_book_v2_header(&header_bytes) { - Ok(header) => CacheHeader::Present(header), - Err(_) => CacheHeader::Unreadable, + CacheHeader::Absent +} + +/// `None` when the named index is not there, `Some(None)` when it is there +/// and unreadable, `Some(Some(header))` when it decodes. +fn read_book_index_header< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + name: &str, +) -> Option> +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let file = match book.open_file_in_dir(name, Mode::ReadOnly) { + Ok(file) => file, + Err(embedded_sdmmc::Error::NotFound) => return None, + // A card that would not open the file is not proof the index is + // missing, and only "missing" licenses a delete — so it reads as + // present-and-unreadable, the distinction this whole function exists + // to make. + Err(_) => return Some(None), + }; + let mut header_bytes = [0u8; BOOK_V2_HEADER_BYTES]; + if read_exact_file(&file, &mut header_bytes).is_err() { + return Some(None); } + Some(decode_book_v2_header(&header_bytes).ok()) +} + +/// Whether the registry file is there at all, for the one caller that has to +/// tell "no cache" from "a cache we cannot interpret". Anything but an +/// outright `NotFound` counts as present, for the same reason as above. +fn registry_present( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, +) -> bool +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + !matches!( + book.open_file_in_dir(CACHE_CONFIG_FILE, Mode::ReadOnly), + Err(embedded_sdmmc::Error::NotFound) + ) } /// Section files deleted per directory-listing pass. embedded-sdmmc will not @@ -1013,6 +1101,7 @@ where CACHE_TOC_FILE, CACHE_COVER_FILE, CACHE_CONTENT_FILE, + CACHE_CONFIG_FILE, ] { if upload_store::remove_file_reclaiming_clusters(&book, name) == upload_store::RemoveStatus::Failed @@ -1020,6 +1109,26 @@ where cleared = false; } } + // The per-config indexes, by listing rather than by name: the + // registry that would name them has just been deleted, and it was + // never the authority on what is on the card anyway. A key past the + // listing budget survives to fail `book_dir_is_reclaimed` below, + // which is what stops this from reporting a clear it did not finish. + match book_index_names_unlisted_by(&book, &LayoutConfigRegistry::new()) { + Some(names) => { + for name in &names { + if upload_store::remove_file_reclaiming_clusters(&book, name.as_str()) + == upload_store::RemoveStatus::Failed + { + cleared = false; + } + } + } + // A listing that would not run leaves indexes this pass never got + // to name. `book_dir_is_reclaimed` below would refuse the clear + // anyway; failing here says so without depending on that. + None => cleared = false, + } match book.open_dir(CACHE_SECTIONS_DIR) { Ok(sections) => { if !empty_sections_dir(§ions) { @@ -1057,30 +1166,21 @@ where cleared } -/// The section ordinal a `S###.BIN` name encodes, or `None` for any other -/// name. Parsed rather than trusted: `SECTIONS/` is on removable media, so a -/// name that is not one of ours must be left alone, not miscounted into the -/// prune range. -fn section_ordinal_from_name(name: &str) -> Option { - let digits = name - .strip_prefix(['S', 's'])? - .get(..3) - .filter(|rest| rest.bytes().all(|byte| byte.is_ascii_digit()))?; - let suffix = name.get(4..)?; - if !suffix.eq_ignore_ascii_case(".BIN") { - return None; - } - digits.parse::().ok() -} - /// Delete the section files a freshly published index no longer names. /// /// Section files are keyed by section *ordinal* — a dense `0..count` counter /// the walk assigns as it flushes, distinct from the spine index the record /// also carries — so a rebuild producing fewer sections than the one before -/// it strands `S..` on the card. Nothing references them: -/// `load_v2_section_by_global_page` indexes off BOOK.BIN. They are unreachable -/// but not free, and they survive until the whole cache directory is emptied. +/// it strands `S..` on the card. Nothing references them: +/// `load_v2_section_by_global_page` indexes off the config's book index. They +/// are unreachable but not free, and they survive until the whole cache +/// directory is emptied. +/// +/// Scoped to one layout config. The ordinal ranges of two resident configs +/// overlap by construction — each numbers its own sections from zero — so a +/// prune that went by ordinal alone would delete the *other* config's +/// pagination past this one's count, which is a working cached copy of the book +/// and the whole point of keeping more than one. /// /// No shipping setting reaches that state today. Sections split on content /// volume — `flush_if_full` breaks on block and text capacity long before the @@ -1109,6 +1209,7 @@ fn prune_orphan_sections_in< const MAX_VOLUMES: usize, >( sections: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + layout_key: u8, keep_count: u16, ) -> usize where @@ -1134,8 +1235,12 @@ where if write!(name, "{}", entry.name).is_err() { return; } - match section_ordinal_from_name(name.as_str()) { - Some(ordinal) if ordinal >= keep_count => { + // Parsed rather than trusted, and matched on the config as well + // as the ordinal: `SECTIONS/` is on removable media, so a name + // that is not one of ours is left alone, and one that is + // another config's is not this prune's to take. + match section_file_name_parts(name.as_str()) { + Some((key, ordinal)) if key == layout_key && ordinal >= keep_count => { let _ = names.push(name); } _ => {} @@ -1178,7 +1283,10 @@ where removed } -/// [`prune_orphan_sections_in`], opening the book's `SECTIONS/` directory. +/// [`prune_orphan_sections_in`], opening the book's `SECTIONS/` directory and +/// taking the layout config from the store whose pagination was just published +/// — the same derivation every other per-config reader and writer uses, so no +/// caller can prune one config's range out of another's files. pub fn prune_orphan_sections< D, T, @@ -1188,14 +1296,16 @@ pub fn prune_orphan_sections< >( root: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, key: &str, + library: &ReaderStore, keep_count: u16, ) -> usize where D: embedded_sdmmc::BlockDevice, T: TimeSource, { + let layout_key = layout_key_for(library); with_v2_sections_dir(root, key, |sections| match sections { - Some(sections) => prune_orphan_sections_in(sections, keep_count), + Some(sections) => prune_orphan_sections_in(sections, layout_key, keep_count), None => 0, }) } @@ -1369,86 +1479,96 @@ where if ensure_v2_cache_dirs(root, key).is_err() { return false; } - with_v2_book_file(root, key, Mode::ReadWriteCreateOrTruncate, |file| { - let toc_count = library - .toc_count - .min(MAX_SD_TOC_ITEMS) - .min(u16::MAX as usize); - let title_text_bytes = library.title.len().min(64) as u32; - let author_text_bytes = library.author.len().min(64) as u32; - let header = BookV2Header { - source_hash: source_identity.0, - source_size: source_identity.1, - total_pages, - section_count: sections.len().min(u16::MAX as usize) as u16, - spine_count: sections - .iter() - .map(|section| section.spine as usize + 1) - .max() - .unwrap_or(0) - .min(u16::MAX as usize) as u16, - toc_count: toc_count as u16, - toc_text_bytes: library - .toc_text_len - .min(MAX_SD_TOC_TEXT_BYTES) - .min(u32::MAX as usize) as u32, - title_text_bytes, - author_text_bytes, - viewport_width: 800, - viewport_height: 480, - font_config: layout::reader_layout_config(library.type_settings(), library.portrait()), - custom_font_identity: library.custom_font_identity(), - partial, - resume_spine, - }; - let mut bytes = [0u8; BOOK_V2_HEADER_BYTES]; - if encode_book_v2_header(header, &mut bytes).is_err() { - return false; - } - let mut stage = WriteStage::new(file); - if stage.push(&bytes).is_err() { - return false; - } - let mut record_bytes = [0u8; BOOK_V2_SECTION_RECORD_BYTES]; - for section in sections { - if encode_book_v2_section(*section, &mut record_bytes).is_err() - || stage.push(&record_bytes).is_err() + let layout_key = layout_key_for(library); + with_v2_book_file( + root, + key, + layout_key, + Mode::ReadWriteCreateOrTruncate, + |file| { + let toc_count = library + .toc_count + .min(MAX_SD_TOC_ITEMS) + .min(u16::MAX as usize); + let title_text_bytes = library.title.len().min(64) as u32; + let author_text_bytes = library.author.len().min(64) as u32; + let header = BookV2Header { + source_hash: source_identity.0, + source_size: source_identity.1, + total_pages, + section_count: sections.len().min(u16::MAX as usize) as u16, + spine_count: sections + .iter() + .map(|section| section.spine as usize + 1) + .max() + .unwrap_or(0) + .min(u16::MAX as usize) as u16, + toc_count: toc_count as u16, + toc_text_bytes: library + .toc_text_len + .min(MAX_SD_TOC_TEXT_BYTES) + .min(u32::MAX as usize) as u32, + title_text_bytes, + author_text_bytes, + viewport_width: 800, + viewport_height: 480, + font_config: layout::reader_layout_config( + library.type_settings(), + library.portrait(), + ), + custom_font_identity: library.custom_font_identity(), + partial, + resume_spine, + }; + let mut bytes = [0u8; BOOK_V2_HEADER_BYTES]; + if encode_book_v2_header(header, &mut bytes).is_err() { + return false; + } + let mut stage = WriteStage::new(file); + if stage.push(&bytes).is_err() { + return false; + } + let mut record_bytes = [0u8; BOOK_V2_SECTION_RECORD_BYTES]; + for section in sections { + if encode_book_v2_section(*section, &mut record_bytes).is_err() + || stage.push(&record_bytes).is_err() + { + return false; + } + } + let mut toc_bytes = [0u8; TOC_RECORD_BYTES]; + for record in library.toc.iter().take(toc_count).copied() { + if encode_toc(record, &mut toc_bytes).is_err() || stage.push(&toc_bytes).is_err() { + return false; + } + } + if stage.flush().is_err() { + return false; + } + if header.toc_text_bytes > 0 + && file + .write(&library.toc_text[..header.toc_text_bytes as usize]) + .is_err() { return false; } - } - let mut toc_bytes = [0u8; TOC_RECORD_BYTES]; - for record in library.toc.iter().take(toc_count).copied() { - if encode_toc(record, &mut toc_bytes).is_err() || stage.push(&toc_bytes).is_err() { + if header.title_text_bytes > 0 + && file + .write(&library.title.as_bytes()[..header.title_text_bytes as usize]) + .is_err() + { return false; } - } - if stage.flush().is_err() { - return false; - } - if header.toc_text_bytes > 0 - && file - .write(&library.toc_text[..header.toc_text_bytes as usize]) - .is_err() - { - return false; - } - if header.title_text_bytes > 0 - && file - .write(&library.title.as_bytes()[..header.title_text_bytes as usize]) - .is_err() - { - return false; - } - if header.author_text_bytes > 0 - && file - .write(&library.author.as_bytes()[..header.author_text_bytes as usize]) - .is_err() - { - return false; - } - true - }) + if header.author_text_bytes > 0 + && file + .write(&library.author.as_bytes()[..header.author_text_bytes as usize]) + .is_err() + { + return false; + } + true + }, + ) .unwrap_or(false) } @@ -1546,48 +1666,59 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { - with_v2_section_file(root, key, section, Mode::ReadOnly, |file| { - let mut header_bytes = [0u8; SECTION_V2_HEADER_BYTES]; - if read_exact_file(file, &mut header_bytes).is_err() { - return CacheLoadResult::Invalid; - } - let Ok(header) = decode_section_v2_header(&header_bytes) else { - return CacheLoadResult::Invalid; - }; - if header.source_hash != source_identity.0 - || header.source_size != source_identity.1 - || header.spine != expected_spine - { - return CacheLoadResult::Invalid; - } - let expected_config = - layout::reader_layout_config(library.type_settings(), library.portrait()); - if header.custom_font_identity != library.custom_font_identity() { - return CacheLoadResult::Invalid; - } - // Cached blocks are pre-wrapped lines: they survive a spacing - // change (heights re-walk below) but not a size change, which - // alters every wrap point and needs the full EPUB rebuild. - if header.font_config & !0b11 != expected_config & !0b11 { - return CacheLoadResult::Invalid; - } - let layout_matches = header.font_config == expected_config; - if !load_v2_section_body(file, header, library) { - return CacheLoadResult::Invalid; - } - if !layout_matches { - layout::rebuild_page_index(library); - } - let pages = library.page_count; - if pages < target_pages { - CacheLoadResult::TooShort { pages } - } else { - CacheLoadResult::Hit { - pages, - repaginated: !layout_matches, + let expected_config = layout::reader_layout_config(library.type_settings(), library.portrait()); + with_v2_section_file( + root, + key, + layout_cache_key(expected_config), + section, + Mode::ReadOnly, + |file| { + let mut header_bytes = [0u8; SECTION_V2_HEADER_BYTES]; + if read_exact_file(file, &mut header_bytes).is_err() { + return CacheLoadResult::Invalid; } - } - }) + let Ok(header) = decode_section_v2_header(&header_bytes) else { + return CacheLoadResult::Invalid; + }; + if header.source_hash != source_identity.0 + || header.source_size != source_identity.1 + || header.spine != expected_spine + { + return CacheLoadResult::Invalid; + } + if header.custom_font_identity != library.custom_font_identity() { + return CacheLoadResult::Invalid; + } + // Cached blocks are pre-wrapped lines: they survive a spacing + // change (heights re-walk below) but not a size change, which + // alters every wrap point and needs the full EPUB rebuild. + // + // The file name already carries the wrap-relevant bits, so what is + // left for this check to catch is a wrap-version or panel-salt bump + // — the axes deliberately kept out of the name, so a bump retires + // every config's files in place rather than stranding them. + if header.font_config & !0b11 != expected_config & !0b11 { + return CacheLoadResult::Invalid; + } + let layout_matches = header.font_config == expected_config; + if !load_v2_section_body(file, header, library) { + return CacheLoadResult::Invalid; + } + if !layout_matches { + layout::rebuild_page_index(library); + } + let pages = library.page_count; + if pages < target_pages { + CacheLoadResult::TooShort { pages } + } else { + CacheLoadResult::Hit { + pages, + repaginated: !layout_matches, + } + } + }, + ) .unwrap_or(CacheLoadResult::Miss) } @@ -1615,6 +1746,7 @@ where with_v2_section_file( root, key, + layout_key_for(library), section, Mode::ReadWriteCreateOrTruncate, |file| write_v2_section_body(file, source_identity, library.cached_spine, library), @@ -1684,7 +1816,7 @@ where T: TimeSource, { let mut name = String::::new(); - section_file_name(section, &mut name); + section_file_name(layout_key_for(library), section, &mut name); match sections.open_file_in_dir(name.as_str(), Mode::ReadWriteCreateOrTruncate) { Ok(file) => write_v2_section_body(&file, source_identity, library.cached_spine, library), Err(_) => { @@ -1720,6 +1852,426 @@ where Some(dir) } +/// The layout config `library` is currently reading under, as the key its +/// cache files are named for. Every per-config reader and writer derives it +/// here rather than taking it as an argument: the store already carries the +/// settings and the page box, and a caller that could pass a different key +/// than the header check compares against would be able to write a file +/// under one config's name holding another's pagination. +fn layout_key_for(library: &ReaderStore) -> u8 { + layout_cache_key(layout::reader_layout_config( + library.type_settings(), + library.portrait(), + )) +} + +/// How many registry-unlisted index files one directory listing will +/// collect. Two configs are resident by design, so a third is already a +/// registry that lost track; past four, whatever wrote them was not this. +const UNLISTED_INDEX_BUDGET: usize = 4; + +/// Read a book's resident-config registry from an already-open book +/// directory. `None` covers both "no registry" and "a registry that says +/// nothing usable" — the caller's response to either is to write the config +/// it is opening, so they need not be told apart. +fn read_layout_registry_in< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, +) -> Option +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let file = book + .open_file_in_dir(CACHE_CONFIG_FILE, Mode::ReadOnly) + .ok()?; + let mut bytes = [0u8; CACHE_CONFIG_BYTES]; + read_exact_file(&file, &mut bytes).ok()?; + decode_layout_config_registry(&bytes).ok() +} + +fn write_layout_registry_in< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + registry: &LayoutConfigRegistry, +) -> bool +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let mut bytes = [0u8; CACHE_CONFIG_BYTES]; + if encode_layout_config_registry(registry, &mut bytes).is_err() { + return false; + } + match book.open_file_in_dir(CACHE_CONFIG_FILE, Mode::ReadWriteCreateOrTruncate) { + Ok(file) => file.write(&bytes).is_ok(), + Err(_) => false, + } +} + +/// What adopting a layout config found waiting for it. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct LayoutConfigAdoption { + /// The registry already listed this config, so a cache built under it + /// should be waiting — which is what makes a flip back to a + /// previously-read type size or orientation instant instead of a full + /// re-wrap. Whether every file really landed is the load's to find out; + /// this is what the registry claims, and it is reported, not relied on. + pub resident: bool, + /// A least-recently-used config whose files this adoption deleted to + /// stay inside [`proto::cache::CACHE_CONFIG_SLOTS`]. Set only once every + /// one of them is gone; a partial delete reports `eviction_failed`. + pub evicted: Option, + /// The least-recently-used config this open had to evict and could not: + /// one of its files refused to delete. Nothing was promoted — the + /// registry still names that config, so it stays counted and the next + /// open under this one retries the same deletes. + pub eviction_failed: Option, + /// Pre-per-config artifacts (an unkeyed `BOOK.BIN`, unkeyed section + /// files) were found and deleted. One-time, on the first open after the + /// firmware that wrote them. + pub purged_legacy: bool, + /// The registry write did not land, so `CFG.BIN` no longer describes which + /// configs are on the card. The open continues — see + /// [`adopt_layout_config`] for why refusing it would be worse — and the + /// next one repairs the registry by listing the index files that are + /// really there, which is what holds the bound. Reported so the log names + /// the card that is failing small writes. + pub registry_write_failed: bool, +} + +/// Claim the layout config `library` is reading under as this book's most +/// recently used, and hold the card to at most +/// [`proto::cache::CACHE_CONFIG_SLOTS`] paginated copies of the book. +/// +/// Runs once per book open, before anything tries to load or build, because +/// it is what both of those need to be true: the registry has to name this +/// config before an index written under it can be found again, and the +/// least recently used config's files have to be gone before a third one's +/// arrive. Adopting before the load rather than after means an open that +/// then fails has evicted for nothing — one rebuild's worth of cost, traded +/// for never having a build write files no reader can find. +/// +/// An eviction that cannot delete every one of that config's files abandons +/// the adoption rather than completing it: the registry is left naming the +/// config whose files are still there. See the failure path below for why +/// that ordering is the one that keeps the bound honest. +pub fn adopt_layout_config< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + root: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + key: &str, + library: &ReaderStore, +) -> LayoutConfigAdoption +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let layout_key = layout_key_for(library); + let mut adoption = LayoutConfigAdoption { + resident: false, + evicted: None, + eviction_failed: None, + purged_legacy: false, + registry_write_failed: false, + }; + // The tree has to exist before the registry can be written into it, and + // this is the earliest point in an open that knows the book's key. A + // failure here fails the build too, so there is nothing else to do. + if ensure_v2_cache_dirs(root, key).is_err() { + cache_log!("cache: adopt config ensure dirs failed key={}", key); + return adoption; + } + let Some(book) = open_v2_book_dir(root, key) else { + return adoption; + }; + adoption.purged_legacy = purge_legacy_artifacts_in(&book); + // A registry file that is there and will not decode gets rebuilt from the + // index files that really are on the card, rather than started over from + // empty. Starting empty is what loses the bound: the configs already there + // stop being counted, so no eviction ever takes them, and every later open + // is free to add another full section set on top. One refused `CFG.BIN` + // write is enough to reach that state — it truncates on open — and the cost + // compounds, because each open that then fails to write leaves one more set + // uncounted. Repairing on read is what makes that self-correcting. + // + // Seeding cannot know which config was most recently used, so the order is + // whatever the listing gave. The config being adopted is promoted to the + // front below either way, and the worst a wrong order does is evict the + // less useful of two survivors. + // + // A registry that is not there at all is left to start empty: that is a + // book being opened for the first time, which is the common case and must + // not pay for a directory listing. A `CFG.BIN` deleted outright while index + // files remain is the one gap left — the clear still sweeps by listing, and + // `read_cache_header`'s fallback still finds the book. + let mut registry = match read_layout_registry_in(&book) { + Some(registry) => registry, + None if registry_present(&book) => layout_registry_from_index_files(&book), + None => LayoutConfigRegistry::new(), + }; + adoption.resident = registry.contains(layout_key); + let evicted = registry.promote(layout_key); + if let Some(evicted) = evicted { + // Delete first, then record the shorter registry: a failure between + // the two leaves files the registry no longer names, which the next + // clear sweeps by listing. Recording first would risk the opposite — + // a registry claiming a config whose files are half gone. + if !delete_layout_config_artifacts_in(&book, evicted) { + // The delete refused, so the promoted registry does not describe + // the card and must not be written. Leaving the stored one alone + // is what keeps the two-config bound honest: it still names the + // config whose files are still there, so eviction keeps counting + // them and the next open under this config retries the same + // deletes. Writing the promotion here would unregister an intact + // cache instead — no later eviction would look at it again, and a + // card that has just refused to give space back would be asked + // for a third set on top of it. + cache_log!( + "cache: config evict failed key={} cfg={} kept={}", + key, + layout_key, + evicted + ); + adoption.eviction_failed = Some(evicted); + return adoption; + } + adoption.evicted = Some(evicted); + } + if !write_layout_registry_in(&book, ®istry) { + cache_log!( + "cache: config registry write failed key={} cfg={}", + key, + layout_key + ); + // Reported rather than fatal. The build below still writes under this + // config and its index is still found — by the next open under the same + // config once the registry is repaired, and by the unlisted-index scan + // meanwhile — so refusing the open here would cost a readable book to + // buy nothing. What the failure does cost is the registry's account of + // the card, and the seeding above is what gets that back. + adoption.registry_write_failed = true; + } + adoption +} + +/// Rebuild a registry from the per-config index files a book directory actually +/// holds, for an open whose `CFG.BIN` is there and unusable. +/// +/// Ordering is lost — a listing cannot say which config was read last — so this +/// only restores the *count*, which is the part eviction needs to hold the +/// two-config bound. A card holding more sets than the bound (which is how it +/// got here) leaves the extras unnamed for now; the eviction the caller runs +/// next brings the count down, and later opens converge. +fn layout_registry_from_index_files< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, +) -> LayoutConfigRegistry +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let mut registry = LayoutConfigRegistry::new(); + // Every keyed index is "unlisted" by an empty registry, so this is the same + // bounded listing `read_cache_header` falls back to. A listing that would + // not run leaves the registry empty, which is where it started. + let Some(names) = book_index_names_unlisted_by(book, ®istry) else { + return registry; + }; + for name in &names { + if let Some(layout_key) = layout_key_of_book_index_file_name(name.as_str()) { + // Seeding, not evicting: nothing is deleted here, so a key pushed + // out of the last slot is simply one this pass could not account + // for. + let _ = registry.promote(layout_key); + } + } + registry +} + +/// Delete one layout config's cache files: its index and every section file +/// named for it. The other configs' files, and everything +/// settings-independent (TOC.BIN, COVER.BIN, CONT.BIN, the position), stay. +/// +/// Returns whether every one of that config's files is now gone — deleted +/// here or already absent. This is the answer the caller's registry write +/// hangs on, so anything it cannot account for reads as false: a delete that +/// refused, a `SECTIONS/` that would not open, a listing that would not +/// finish. The index goes first, so a sweep that stops partway leaves an +/// unreadable set (stray sections, no index) rather than an index promising +/// sections that are gone. +fn delete_layout_config_artifacts_in< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + layout_key: u8, +) -> bool +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let mut name = String::::new(); + book_index_file_name(layout_key, &mut name); + if upload_store::remove_file_reclaiming_clusters(book, name.as_str()) + == upload_store::RemoveStatus::Failed + { + return false; + } + match book.open_dir(CACHE_SECTIONS_DIR) { + Ok(sections) => sweep_section_files(§ions, |name| { + layout_key_of_section_file_name(name) == Some(layout_key) + }), + // No `SECTIONS/` is the end state this wants, reached without it. + Err(embedded_sdmmc::Error::NotFound) => true, + // A directory that would not open may still hold the config's files. + Err(_) => false, + } +} + +/// Delete the artifacts of the single-config scheme this replaced: the +/// unkeyed `BOOK.BIN` and the unkeyed `S.BIN` section files. +/// +/// Gated on `BOOK.BIN` being there, which is the marker: nothing writes it +/// anymore, so its presence is the only reason to pay for a sections listing. +/// The marker is therefore deleted only once the sections sweep reports itself +/// finished — it is the whole retry mechanism, and a purge that took it first +/// would leave anything it could not delete with nothing to bring a later open +/// back for it. +/// +/// Returns whether there was anything to purge — not whether all of it went. +/// Unlike eviction, no caller needs the stronger answer: no registry ever named +/// these files, so what refuses to go is retried by the next open, which still +/// finds `BOOK.BIN`, and swept by the next clear if that one runs first. +fn purge_legacy_artifacts_in< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, +) -> bool +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + if book + .open_file_in_dir(CACHE_BOOK_FILE, Mode::ReadOnly) + .is_err() + { + return false; + } + // Sections first, the marker last. `BOOK.BIN` is the only thing that brings + // a later open back here, so taking it before the sweep has finished is + // what would strand whatever the sweep could not: nothing reads the unkeyed + // names, no registry counts them, and no later pass would look again. + // Leaving the marker costs one failed open per later open and buys the + // retry. Nothing reads `BOOK.BIN` itself anymore, so a marker outliving its + // sections misleads no reader. + let swept = match book.open_dir(CACHE_SECTIONS_DIR) { + Ok(sections) => sweep_section_files(§ions, is_legacy_section_file_name), + // No `SECTIONS/` is the end state the sweep wants, reached without it. + Err(embedded_sdmmc::Error::NotFound) => true, + Err(_) => false, + }; + if swept { + let _ = upload_store::remove_file_reclaiming_clusters(book, CACHE_BOOK_FILE); + } + true +} + +/// Delete every file in an open `SECTIONS/` whose name `wanted` accepts, in +/// the same bounded batches as [`empty_sections_dir`]: names are collected +/// per pass and deleted with the listing closed, because deleting while +/// iterating is what the directory walk cannot survive. +/// +/// Returns whether a listing came back with nothing left for `wanted` — the +/// only evidence that the sweep is finished. A refused delete, a listing that +/// would not run, a name that could not be read back to test, or a pass +/// budget spent before the directory came back clean all read as false; the +/// caller must not report the files gone on any of them. +fn sweep_section_files< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + sections: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + wanted: impl Fn(&str) -> bool, +) -> bool +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + use core::fmt::Write; + let max_passes = MAX_BOOK_SECTIONS.div_ceil(SECTION_SWEEP_BATCH) + 1; + for _ in 0..max_passes { + let mut names: heapless::Vec, SECTION_SWEEP_BATCH> = + heapless::Vec::new(); + // Something this pass could not put to `wanted`. A full batch is not + // that — the next pass re-lists what it left — but a name that would + // not read back is: it may be one of the files being swept, and a + // sweep cannot claim what it never got to test. + let mut blocked = false; + if sections + .iterate_dir(|entry| { + if names.is_full() || entry.attributes.is_directory() { + return; + } + let mut name = String::::new(); + if write!(name, "{}", entry.name).is_err() { + blocked = true; + return; + } + if wanted(name.as_str()) && names.push(name).is_err() { + blocked = true; + } + }) + .is_err() + { + return false; + } + if names.is_empty() { + return !blocked; + } + for name in &names { + if upload_store::remove_file_reclaiming_clusters(sections, name.as_str()) + == upload_store::RemoveStatus::Failed + { + // A refusal will repeat on the next pass and spend the whole + // budget re-finding the same file. Leaving it costs SD space + // until the next clear, which lists rather than names. + return false; + } + } + } + false +} + /// Open the book's `CONT.BIN` (settings-independent content cache) and run /// `f` with it. pub fn with_v2_content_file< @@ -2035,6 +2587,12 @@ where /// the content replay path runs precisely when the index is layout-invalid, /// but its TOC and labels are settings-independent and must survive into /// the rewritten index. Deliberately does not touch the section index. +/// +/// "Any config" is now literal: the index this wants belongs to whichever +/// config the book was last read under, not the one being built, so it works +/// through the registry's slots in order. Slot 0 is the config being adopted +/// and its index is exactly the one that just missed, so in practice the +/// answer comes from the slot behind it. pub fn load_v2_book_labels_and_toc< D, T, @@ -2051,28 +2609,23 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { - with_v2_book_file(root, key, Mode::ReadOnly, |file| { + with_any_v2_book_file(root, key, true, |file| { let mut header_bytes = [0u8; BOOK_V2_HEADER_BYTES]; - if read_exact_file(file, &mut header_bytes).is_err() { - return false; - } - let Ok(header) = decode_book_v2_header(&header_bytes) else { - return false; - }; + read_exact_file(file, &mut header_bytes).ok()?; + let header = decode_book_v2_header(&header_bytes).ok()?; if header.source_hash != source_identity.0 || header.source_size != source_identity.1 || header.section_count as usize > MAX_BOOK_SECTIONS || !v2_toc_label_bounds_ok(&header) { - return false; + return None; } let toc_offset = BOOK_V2_HEADER_BYTES + header.section_count as usize * BOOK_V2_SECTION_RECORD_BYTES; - if file.seek_from_start(toc_offset as u32).is_err() { - return false; - } - read_v2_toc_into_library(file, &header, library) - && read_v2_labels_into_library(file, &header, library) + file.seek_from_start(toc_offset as u32).ok()?; + (read_v2_toc_into_library(file, &header, library) + && read_v2_labels_into_library(file, &header, library)) + .then_some(true) }) .unwrap_or(false) } @@ -2087,6 +2640,7 @@ fn with_v2_section_file< >( root: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, key: &str, + layout_key: u8, spine: u16, mode: Mode, f: impl for<'a> FnOnce(&File<'a, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>) -> R, @@ -2100,7 +2654,7 @@ where let book_dir = cache.open_dir(key).ok()?; let sections = book_dir.open_dir(CACHE_SECTIONS_DIR).ok()?; let mut name = String::::new(); - section_file_name(spine, &mut name); + section_file_name(layout_key, spine, &mut name); let file = sections.open_file_in_dir(name.as_str(), mode).ok()?; Some(f(&file)) } @@ -2115,6 +2669,7 @@ fn with_v2_book_file< >( root: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, key: &str, + layout_key: u8, mode: Mode, f: impl for<'a> FnOnce(&File<'a, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>) -> R, ) -> Option @@ -2125,10 +2680,116 @@ where let xteink = root.open_dir(CACHE_ROOT_DIR).ok()?; let cache = xteink.open_dir(CACHE_V2_DIR).ok()?; let book_dir = cache.open_dir(key).ok()?; - let file = book_dir.open_file_in_dir(CACHE_BOOK_FILE, mode).ok()?; + let mut name = String::::new(); + book_index_file_name(layout_key, &mut name); + let file = book_dir.open_file_in_dir(name.as_str(), mode).ok()?; Some(f(&file)) } +/// Run `f` against whichever of the book's per-config indexes is there, +/// for the readers after facts that do not depend on the layout config: the +/// source identity, the title, the TOC. They used to open one fixed +/// `BOOK.BIN`; now the index is per config, so "the book's index" means +/// asking the registry which configs are resident and taking the first that +/// opens. `f` returning `None` means "this index did not answer" and moves +/// on to the next. +/// +/// `scan_if_unlisted` covers the registry being lost while index files +/// survive: a bounded directory listing finds them by name. Callers on a +/// scan-time path leave it off, because for them a miss costs only a +/// fallback label, not a cache. +fn with_any_v2_book_file< + R, + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + root: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + key: &str, + scan_if_unlisted: bool, + mut f: impl for<'a> FnMut(&File<'a, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>) -> Option, +) -> Option +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let book_dir = open_v2_book_dir(root, key)?; + let mut name = String::::new(); + let registry = read_layout_registry_in(&book_dir).unwrap_or_default(); + for layout_key in registry.keys() { + book_index_file_name(*layout_key, &mut name); + if let Ok(file) = book_dir.open_file_in_dir(name.as_str(), Mode::ReadOnly) { + if let Some(answer) = f(&file) { + return Some(answer); + } + } + } + if !scan_if_unlisted { + return None; + } + // A listing that would not run reads as nothing left to try. Every caller + // here is after a config-independent fact and treats `None` as a miss it + // has a fallback for — unlike `read_cache_header`, whose own loop must tell + // a failed listing from an empty directory because a delete hangs on it. + for unlisted in book_index_names_unlisted_by(&book_dir, ®istry).unwrap_or_default() { + if let Ok(file) = book_dir.open_file_in_dir(unlisted.as_str(), Mode::ReadOnly) { + if let Some(answer) = f(&file) { + return Some(answer); + } + } + } + None +} + +/// The per-config index files present in a book's cache directory that the +/// registry does not name — what is left to try when the registry was lost +/// or never written. Bounded by [`CACHE_CONFIG_SLOTS`]-plus-slack rather +/// than by the 64 keys a name could carry: past that, whatever wrote them +/// was not this firmware. +fn book_index_names_unlisted_by< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + registry: &LayoutConfigRegistry, +) -> Option, UNLISTED_INDEX_BUDGET>> +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + use core::fmt::Write; + let mut found = heapless::Vec::new(); + // `None` on a listing that would not run, never an empty result. The caller + // reads "nothing here" as licence to delete the directory, and a card that + // refused to enumerate has not said that. + if book + .iterate_dir(|entry| { + if found.is_full() || entry.attributes.is_directory() { + return; + } + let mut name = String::::new(); + if write!(name, "{}", entry.name).is_err() { + return; + } + let Some(layout_key) = layout_key_of_book_index_file_name(name.as_str()) else { + return; + }; + if !registry.contains(layout_key) { + let _ = found.push(name); + } + }) + .is_err() + { + return None; + } + Some(found) +} + fn with_v2_toc_file< R, D, diff --git a/reader-cache/src/publish.rs b/reader-cache/src/publish.rs index 9330a71e..696f36b2 100644 --- a/reader-cache/src/publish.rs +++ b/reader-cache/src/publish.rs @@ -147,10 +147,15 @@ where // frontier rather than the book's real length; pruning against it would // delete the sections that walk is about to need. Only a completed // build — which stamps zero — knows the final count. + // + // The count is this layout config's, so the prune is too: it takes the key + // from `library`, the same store whose pagination was just written, and the + // other resident config's sections are none of its business. if resume_spine == 0 { let pruned = files::prune_orphan_sections( root, cache_key, + library, sections_slice.len().min(u16::MAX as usize) as u16, ); if pruned > 0 { diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index cbfba88b..b7510522 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -17,18 +17,26 @@ //! `upload-store`'s public API to avoid ~120 duplicated lines would put test //! scaffolding in a shipped crate. If a third crate ever needs it, that is the //! point to extract it into a dev-only crate of its own. +//! +//! The last section covers the per-config cache layout (B7) rather than a +//! fault: which files a layout config owns, that a second config does not +//! overwrite the first, and which one eviction takes. It lives here for the +//! card harness above — a second integration test would have to copy all of +//! it, which is the cost this file's header already argues against. use std::cell::{Cell, RefCell}; use std::rc::Rc; -use display::font::FontStyle; +use display::font::{FontSize, FontStyle, TypeSettings}; use embedded_sdmmc::{ Block, BlockCount, BlockDevice, BlockIdx, Directory, TimeSource, Timestamp, VolumeIdx, VolumeManager, }; use proto::cache::{ - BookV2SectionRecord, CoverCacheHeader, CACHE_COVER_FILE, COVER_BYTES, COVER_HEIGHT, - COVER_STRIDE, COVER_WIDTH, + book_index_file_name, layout_cache_key, section_file_name, BookV2SectionRecord, + CoverCacheHeader, BOOK_INDEX_FILE_BYTES, CACHE_BOOK_FILE, CACHE_CONFIG_FILE, CACHE_COVER_FILE, + CACHE_SECTIONS_DIR, CACHE_SECTION_FILE_BYTES, COVER_BYTES, COVER_HEIGHT, COVER_STRIDE, + COVER_WIDTH, }; use proto::text::{TextAlign, TextRole}; use reader_cache::files::{self, CacheLoadResult}; @@ -297,11 +305,25 @@ fn total_pages(records: &[BookV2SectionRecord]) -> u32 { /// Whether the book's cache directory still holds its section files. This is /// the question the round-2 finding turned on: a failing publish must not take /// the files out from under a reader who is reading them. -fn sections_still_on_card(root: &Dir<'_>, count: usize) -> bool { - (0..count).all(|n| file_in_sections_dir(root, &format!("S{n:03}.BIN"))) +fn sections_still_on_card(root: &Dir<'_>, store: &ReaderStore, count: usize) -> bool { + // Named for the store's layout config, the way the writer names them: a + // section file belongs to one paginated copy of the book, so a helper + // spelling the name itself would pass while the config key drifted. + let layout_key = layout_cache_key_for(store); + (0..count).all(|n| section_file_present(root, layout_key, n as u16)) +} + +/// The per-config cache key for a store's current type settings and page box. +fn layout_cache_key_for(store: &ReaderStore) -> u8 { + layout_cache_key(layout::reader_layout_config( + store.type_settings(), + store.portrait(), + )) } -/// Whether one named file exists in the book's `SECTIONS/` directory. +/// Whether one named file exists in the book's `SECTIONS/` directory. Takes a +/// raw name, for the cases that are deliberately not this config's: a stray +/// file, or another config's section. fn file_in_sections_dir(root: &Dir<'_>, name: &str) -> bool { files::with_v2_sections_dir(root, KEY, |sections| { let Some(sections) = sections else { @@ -436,7 +458,7 @@ fn a_failed_final_index_write_restores_the_reader_and_keeps_their_sections() { "the walk must report the failure rather than claim it finished" ); assert!( - sections_still_on_card(&root, 3), + sections_still_on_card(&root, &store, 3), "the reader's section files must survive a failed index write" ); assert_eq!( @@ -470,7 +492,7 @@ fn a_failed_first_open_index_write_clears_the_cache_nobody_is_holding() { ); assert_eq!(result, Err(PublishError::IndexWrite)); assert!( - !sections_still_on_card(&root, 3), + !sections_still_on_card(&root, &store, 3), "an open that never became readable should leave no debris" ); } @@ -625,7 +647,7 @@ fn a_clean_publish_leaves_the_requested_page_resident() { "the requested page must be renderable from RAM after a clean publish" ); assert!( - sections_still_on_card(&root, 3), + sections_still_on_card(&root, &store, 3), "a clean publish must not delete anything" ); } @@ -661,11 +683,11 @@ fn a_rebuild_with_fewer_sections_prunes_the_stranded_tail() { ); assert_eq!(outcome.outcome, BookPublishOutcome::Ready); store.finish_book_load(0, 0, BookLoadStatus::Ready); - assert!(sections_still_on_card(&root, 5)); + assert!(sections_still_on_card(&root, &store, 5)); // A completed rebuild over the same content with a smaller final section // set, as a change to the capacity constants would produce: it rewrites - // S000..S002 and leaves S003 and S004 behind. + // S000..S002 and leaves S003 and S004 behind. let narrow = &wide[..3]; store.begin_book_load(); let outcome = publish::publish_book_cache( @@ -683,11 +705,12 @@ fn a_rebuild_with_fewer_sections_prunes_the_stranded_tail() { store.finish_book_load(0, 0, BookLoadStatus::Ready); assert!( - sections_still_on_card(&root, 3), + sections_still_on_card(&root, &store, 3), "the sections the new index names must survive" ); + let layout_key = layout_cache_key_for(&store); assert!( - !file_in_sections_dir(&root, "S003.BIN") && !file_in_sections_dir(&root, "S004.BIN"), + !section_file_present(&root, layout_key, 3) && !section_file_present(&root, layout_key, 4), "the sections the new index no longer names must be gone" ); } @@ -727,7 +750,7 @@ fn a_suspended_walk_keeps_the_sections_past_its_frontier() { store.finish_book_load(0, 0, BookLoadStatus::Ready); assert!( - sections_still_on_card(&root, 5), + sections_still_on_card(&root, &store, 5), "a provisional publish must leave every section the walk will resume into" ); } @@ -746,6 +769,11 @@ fn the_prune_leaves_names_it_does_not_recognise() { write_stray_file(&root, "NOTES.TXT"); // Two digits, so not a name this code ever writes. write_stray_file(&root, "S12.BIN"); + // A name the *previous* scheme wrote: eight characters, no config key. Its + // ordinal is in the range being pruned, so a prune reading ordinals without + // the key would take it. Retiring these is the legacy purge's job, which is + // gated on BOOK.BIN and knows to look; the prune must leave them. + write_stray_file(&root, "S003.BIN"); let narrow = &wide[..1]; store.begin_book_load(); @@ -764,13 +792,17 @@ fn the_prune_leaves_names_it_does_not_recognise() { store.finish_book_load(0, 0, BookLoadStatus::Ready); assert!( - !file_in_sections_dir(&root, "S001.BIN"), + !section_file_present(&root, layout_cache_key_for(&store), 1), "the orphaned section should still go" ); assert!( file_in_sections_dir(&root, "NOTES.TXT") && file_in_sections_dir(&root, "S12.BIN"), "names this code does not write must be left alone" ); + assert!( + file_in_sections_dir(&root, "S003.BIN"), + "a legacy unkeyed name is not this prune's to take" + ); } /// Invariant: one refused delete does not strand the orphans behind it. @@ -797,13 +829,14 @@ fn a_refused_delete_does_not_strand_the_orphans_behind_it() { // three land in one listing batch (SECTION_SWEEP_BATCH is 16), so this // exercises "kept going after the failure", not "picked it up next pass". build_book(&root, &mut store, 5); - assert!(sections_still_on_card(&root, 5)); + assert!(sections_still_on_card(&root, &store, 5)); + let layout_key = layout_cache_key_for(&store); disk.fault.fail_write_in.set(Some(0)); - let removed = files::prune_orphan_sections(&root, KEY, 2); + let removed = files::prune_orphan_sections(&root, KEY, &store, 2); assert!( - file_in_sections_dir(&root, "S000.BIN") && file_in_sections_dir(&root, "S001.BIN"), + section_file_present(&root, layout_key, 0) && section_file_present(&root, layout_key, 1), "the sections the index still names must survive" ); assert_eq!( @@ -811,13 +844,75 @@ fn a_refused_delete_does_not_strand_the_orphans_behind_it() { "every orphan must come off the card, including the one behind the refused write" ); assert!( - !file_in_sections_dir(&root, "S002.BIN") - && !file_in_sections_dir(&root, "S003.BIN") - && !file_in_sections_dir(&root, "S004.BIN"), + !section_file_present(&root, layout_key, 2) + && !section_file_present(&root, layout_key, 3) + && !section_file_present(&root, layout_key, 4), "a refused delete must not strand the orphans after it" ); } +/// Invariant: the prune stays inside the config that was just published. +/// +/// This is the invariant the per-config layout creates and the prune could +/// silently break. Two resident configs both number their sections from zero, so +/// their ordinal ranges overlap completely: a prune that went by ordinal alone +/// would delete the other config's tail past this one's count — a working +/// paginated copy of the book, and the whole reason for keeping two. +#[test] +fn the_prune_leaves_the_other_config_alone() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + // The config that keeps five sections, and stays untouched throughout. + let keeper_key = build_under_current_config(&root, &mut store, 5); + + // A second config publishes a shorter book, which prunes its own tail. + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let shrinking_key = layout_cache_key_for(&store); + assert_ne!( + shrinking_key, keeper_key, + "the flip must land on a different config for this to test anything" + ); + files::adopt_layout_config(&root, KEY, &store); + let wide = build_book(&root, &mut store, 5); + let narrow = &wide[..2]; + store.begin_book_load(); + let outcome = publish::publish_book_cache( + &root, + KEY, + IDENTITY, + 0, + &mut store, + narrow, + total_pages(narrow), + false, + 0, + ); + assert_eq!(outcome.outcome, BookPublishOutcome::Ready); + store.finish_book_load(0, 0, BookLoadStatus::Ready); + + for section in 2..5 { + assert!( + !section_file_present(&root, shrinking_key, section), + "section {section} of the shrinking config is stranded and must go" + ); + assert!( + section_file_present(&root, keeper_key, section), + "section {section} belongs to the other config and must survive" + ); + } + for section in 0..2 { + assert!( + section_file_present(&root, shrinking_key, section) + && section_file_present(&root, keeper_key, section), + "section {section} is named by both configs' indexes" + ); + } +} + /// Invariant: a progressive first open adopts the cover, like every other /// successful publish. /// @@ -933,7 +1028,7 @@ fn a_step_past_the_batching_threshold_publishes_and_survives_a_refused_write() { "a refused index write must be reported, not swallowed" ); assert!( - sections_still_on_card(&root, sections), + sections_still_on_card(&root, &store, sections), "a refused index write must not take the reader's sections with it" ); assert!( @@ -941,3 +1036,771 @@ fn a_step_past_the_batching_threshold_publishes_and_survives_a_refused_write() { "the reader's page must still be resident after the refused write" ); } + +// --------------------------------------------------------------------------- +// Per-config caches (B7) +// +// A wrap-relevant settings change — type size, weight, family, or the +// portrait/landscape page box — used to overwrite the book's one cached +// pagination, so flipping back re-paid the whole rebuild (24-27 s on the +// measured 11.7 MB book). These pin that each config owns its own files, that +// the registry orders them by use, and that eviction takes the least recently +// used one and nothing else. +// --------------------------------------------------------------------------- + +/// The store's layout config, and a second one that differs only in the page +/// box — the orientation flip, which is the flow that made B7 worth doing. +fn landscape_of(store: &ReaderStore) -> (TypeSettings, bool) { + (store.type_settings(), !store.portrait()) +} + +/// A third config: a type-size change, the other flow that re-paid a rebuild. +fn larger_of(store: &ReaderStore) -> (TypeSettings, bool) { + let mut settings = store.type_settings(); + settings.size = match settings.size { + FontSize::Large => FontSize::Small, + _ => FontSize::Large, + }; + (settings, store.portrait()) +} + +fn section_file_present(root: &Dir<'_>, layout_key: u8, section: u16) -> bool { + files::with_v2_sections_dir(root, KEY, |sections| { + let Some(sections) = sections else { + return false; + }; + let mut name = heapless::String::::new(); + section_file_name(layout_key, section, &mut name); + sections + .open_file_in_dir(name.as_str(), embedded_sdmmc::Mode::ReadOnly) + .is_ok() + }) +} + +fn book_index_present(root: &Dir<'_>, layout_key: u8) -> bool { + let dir = files::open_v2_book_dir(root, KEY).expect("book cache dir"); + let mut name = heapless::String::::new(); + book_index_file_name(layout_key, &mut name); + // Bound rather than returned directly: the `Result` holds a `File` borrowing + // `dir`, and as a tail expression that temporary would outlive `dir`. + let present = dir + .open_file_in_dir(name.as_str(), embedded_sdmmc::Mode::ReadOnly) + .is_ok(); + present +} + +/// Build and publish the whole book under the store's current layout config, +/// the way an open that misses does: adopt the config, write its sections, +/// write its index. Returns the config's cache key. +fn build_under_current_config(root: &Dir<'_>, store: &mut ReaderStore, sections: usize) -> u8 { + let layout_key = layout_cache_key_for(store); + files::adopt_layout_config(root, KEY, store); + let records = build_book(root, store, sections); + let pages = total_pages(&records); + assert!( + files::write_v2_book_index(root, KEY, IDENTITY, pages, &records, store, false, 0), + "the index for config {layout_key:#04x} should write" + ); + store.begin_book_load(); + store.set_book_index(pages, false, &records); + store.finish_book_load(0, 0, BookLoadStatus::Ready); + layout_key +} + +/// Invariant: two layout configs keep separate caches, so returning to one +/// already built is a hit rather than a rebuild. This is B7's whole point. +#[test] +fn a_second_layout_config_does_not_overwrite_the_first() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let portrait_key = build_under_current_config(&root, &mut store, 3); + + // Flip the page box, the way an orientation toggle does, and build again. + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let landscape_key = build_under_current_config(&root, &mut store, 3); + assert_ne!( + portrait_key, landscape_key, + "an orientation flip must land on a different config key, or the caches still collide" + ); + + // Both configs' files are on the card at once -- the state the + // single-file scheme could never hold. + for key in [portrait_key, landscape_key] { + assert!( + book_index_present(&root, key), + "index {key:#04x} must survive" + ); + for section in 0..3 { + assert!( + section_file_present(&root, key, section), + "section {section} of config {key:#04x} must survive" + ); + } + } + + // Flip back: the config is resident, and its index and section load + // without any rebuild. + let (settings, portrait) = (store.type_settings(), !store.portrait()); + store.set_layout(settings, portrait); + let adoption = files::adopt_layout_config(&root, KEY, &store); + assert!( + adoption.resident, + "the config just flipped away from must read as already built" + ); + assert_eq!(adoption.evicted, None, "two configs fit; nothing should go"); + assert!( + matches!( + files::load_v2_book_index(&root, KEY, IDENTITY, &mut store), + files::BookIndexLoadResult::Hit { unfinished: false } + ), + "the flipped-back config's index must load, not miss" + ); + assert!( + matches!( + files::load_v2_section_by_global_page(&root, KEY, IDENTITY, 0, &mut store), + CacheLoadResult::Hit { .. } + ), + "the flipped-back config's section must load from its own file" + ); +} + +/// Invariant: a third config evicts the least recently used one, and takes +/// only its files -- not the surviving config's, and not the +/// settings-independent ones. +#[test] +fn a_third_layout_config_evicts_the_least_recently_used_one() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let first_key = build_under_current_config(&root, &mut store, 2); + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let second_key = build_under_current_config(&root, &mut store, 2); + // COVER.BIN is settings-independent; eviction must not reach it. + write_cover(&root); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let third_key = layout_cache_key_for(&store); + let adoption = files::adopt_layout_config(&root, KEY, &store); + assert_eq!( + adoption.evicted, + Some(first_key), + "the third config must evict the least recently used one" + ); + assert!( + !adoption.resident, + "a config never built cannot be resident" + ); + assert_eq!( + adoption.eviction_failed, None, + "an eviction that took every file has nothing to report as left behind" + ); + + assert!( + !book_index_present(&root, first_key), + "the evicted config's index must be gone" + ); + for section in 0..2 { + assert!( + !section_file_present(&root, first_key, section), + "the evicted config's section {section} must be gone" + ); + assert!( + section_file_present(&root, second_key, section), + "the surviving config's section {section} must not be collateral" + ); + } + assert!( + book_index_present(&root, second_key), + "the surviving config's index must not be collateral" + ); + assert_ne!(third_key, first_key); + assert_eq!( + files::load_v2_cover_cache(&root, KEY, &mut store), + files::CoverLoadResult::Hit, + "COVER.BIN does not depend on the layout config and must survive eviction" + ); +} + +/// Invariant: an eviction that cannot delete the config's index leaves that +/// config registered, and the next open retries it. +/// +/// The registry is the only thing that counts a paginated copy of the book +/// toward the two-config bound. Promoting over a config whose files are still +/// there would unregister a full section set: no later eviction would look at +/// it again, and on a card refusing deletes for want of space the build this +/// open is about to run would be asked for a third set on top of it. +/// +/// The delete is blocked here by holding the file open, which this +/// embedded-sdmmc rev refuses to reopen — the same `RemoveStatus::Failed` an +/// I/O fault produces, but aimed at one named file instead of the Nth write. +#[test] +fn an_eviction_that_cannot_delete_keeps_the_evicted_config_registered() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let first_key = build_under_current_config(&root, &mut store, 2); + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let second_key = build_under_current_config(&root, &mut store, 2); + + // A third config arrives with the least recently used one's index + // undeletable. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let mut index_name = heapless::String::::new(); + book_index_file_name(first_key, &mut index_name); + let held = book + .open_file_in_dir(index_name.as_str(), embedded_sdmmc::Mode::ReadOnly) + .expect("hold the evicted config's index open"); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let blocked = files::adopt_layout_config(&root, KEY, &store); + assert_eq!( + blocked.evicted, None, + "nothing was deleted, so nothing may be reported evicted" + ); + assert_eq!( + blocked.eviction_failed, + Some(first_key), + "the config that would not go must be named, not silently dropped" + ); + + drop(held); + drop(book); + assert!( + book_index_present(&root, first_key), + "the refused delete leaves the index on the card" + ); + for section in 0..2 { + assert!( + section_file_present(&root, first_key, section), + "the index refusal stops the sweep, so section {section} stays too" + ); + assert!( + section_file_present(&root, second_key, section), + "the surviving config is untouched either way" + ); + } + + // The whole point of leaving the registry alone: the config is still + // named, so the next open under the third config evicts it rather than + // walking past files nothing counts. + let retry = files::adopt_layout_config(&root, KEY, &store); + assert_eq!( + retry.evicted, + Some(first_key), + "the registry must still have named the config it could not delete" + ); + assert_eq!(retry.eviction_failed, None); + assert!( + !book_index_present(&root, first_key), + "the retry takes the index the first attempt could not" + ); + for section in 0..2 { + assert!( + !section_file_present(&root, first_key, section), + "the retry takes section {section} as well" + ); + } +} + +/// Invariant: the same holds when it is a *section* file that will not go, +/// after the index has already been deleted. +/// +/// This is the half-deleted case: the config's index is gone, so nothing can +/// read it, but its section files still occupy the card. Reporting that as an +/// eviction would leave those files uncounted forever; keeping the config +/// registered is what sends the next open back for them. +#[test] +fn an_eviction_that_cannot_delete_a_section_file_reports_no_eviction() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let first_key = build_under_current_config(&root, &mut store, 2); + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + build_under_current_config(&root, &mut store, 2); + + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); + let mut section_name = heapless::String::::new(); + section_file_name(first_key, 1, &mut section_name); + let held = sections + .open_file_in_dir(section_name.as_str(), embedded_sdmmc::Mode::ReadOnly) + .expect("hold one of the evicted config's sections open"); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let blocked = files::adopt_layout_config(&root, KEY, &store); + assert_eq!( + blocked.evicted, None, + "a sweep that left a file behind is not an eviction" + ); + assert_eq!(blocked.eviction_failed, Some(first_key)); + + drop(held); + drop(sections); + drop(book); + assert!( + !book_index_present(&root, first_key), + "the index goes first, and that part did succeed" + ); + assert!( + section_file_present(&root, first_key, 1), + "the section that refused to go is still on the card" + ); + + let retry = files::adopt_layout_config(&root, KEY, &store); + assert_eq!( + retry.evicted, + Some(first_key), + "a config whose index is already gone is still the one owed a sweep" + ); + assert!( + !section_file_present(&root, first_key, 1), + "the retry finishes the sweep the refusal interrupted" + ); +} + +/// Invariant: re-reading a resident config moves it off the eviction block. +/// Without this the registry would be insertion-ordered, and the config the +/// reader had just come back to would be the next one thrown away. +#[test] +fn re_reading_a_config_keeps_it_off_the_eviction_block() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let first_key = build_under_current_config(&root, &mut store, 1); + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let second_key = build_under_current_config(&root, &mut store, 1); + + // Go back to the first config without building: it becomes most recent. + let (settings, portrait) = (store.type_settings(), !store.portrait()); + store.set_layout(settings, portrait); + assert!(files::adopt_layout_config(&root, KEY, &store).resident); + + // A third config now takes the *other* one's slot. + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + assert_eq!( + files::adopt_layout_config(&root, KEY, &store).evicted, + Some(second_key), + "eviction must take the config that has not been read since, not the one just re-read" + ); + assert!( + book_index_present(&root, first_key), + "the re-read config must still be on the card" + ); +} + +/// Invariant: the first open of a cache written by the single-config firmware +/// deletes its files, and leaves the settings-independent ones alone. +/// +/// Without this the old `BOOK.BIN` and unkeyed `S.BIN` files would sit +/// on the card forever: nothing reads them under the new names, and the +/// registry that drives eviction never learns they exist. +#[test] +fn the_first_open_after_the_single_config_scheme_purges_its_files() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + // A cache exactly as the previous firmware left it: an unkeyed index, an + // unkeyed section file, and a cover. + files::ensure_v2_cache_dirs(&root, KEY).expect("cache dirs"); + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + for name in [CACHE_BOOK_FILE, "S000.BIN"] { + let dir = if name == CACHE_BOOK_FILE { + None + } else { + Some(book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir")) + }; + let target = dir.as_ref().unwrap_or(&book); + let file = target + .open_file_in_dir(name, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) + .expect("legacy file"); + file.write(&[0u8; 64]).expect("legacy file body"); + } + write_cover(&root); + drop(book); + + let adoption = files::adopt_layout_config(&root, KEY, &store); + assert!( + adoption.purged_legacy, + "an unkeyed BOOK.BIN must be recognized as the previous scheme's" + ); + assert!( + !adoption.resident, + "the purged cache cannot count as this config's" + ); + + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + assert!( + book.open_file_in_dir(CACHE_BOOK_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "the unkeyed index must be gone" + ); + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); + assert!( + sections + .open_file_in_dir("S000.BIN", embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "the unkeyed section file must be gone" + ); + drop(sections); + drop(book); + assert_eq!( + files::load_v2_cover_cache(&root, KEY, &mut store), + files::CoverLoadResult::Hit, + "the purge must not reach the settings-independent files" + ); + + // A second open has nothing left to find, so the pass costs one failed + // open rather than a sections listing. + assert!(!files::adopt_layout_config(&root, KEY, &store).purged_legacy); +} + +/// Invariant: a cache whose registry was lost still identifies its book. +/// +/// `read_cache_header`'s `Absent` is what licenses the orphan sweep to delete +/// a cache directory. The source identity now lives inside a per-config index +/// the registry names, so a lost registry must not be able to make a cache +/// that is plainly there read as absent. +#[test] +fn a_cache_whose_registry_is_lost_still_names_its_book() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + build_under_current_config(&root, &mut store, 2); + match files::read_cache_header(&root, KEY) { + files::CacheHeader::Present(header) => { + assert_eq!((header.source_hash, header.source_size), IDENTITY) + } + other => panic!("a published cache must identify its book, got {other:?}"), + } + + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + book.delete_file_in_dir(CACHE_CONFIG_FILE) + .expect("registry deletes"); + drop(book); + + match files::read_cache_header(&root, KEY) { + files::CacheHeader::Present(header) => { + assert_eq!( + (header.source_hash, header.source_size), + IDENTITY, + "the index is still on the card and still says whose it is" + ) + } + other => panic!("a lost registry must not make a live cache deletable, got {other:?}"), + } +} + +/// Invariant: a corrupt registry reads as `Unreadable`, never `Absent`. +/// +/// `Absent` is what licenses the orphan sweep to delete a cache directory, so +/// a registry that is plainly there but says nothing usable must fail closed: +/// the clear path refuses to delete against a key it cannot prove, and the +/// sweep's own comment turns on the same distinction. +#[test] +fn a_registry_that_says_nothing_usable_fails_closed() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + build_under_current_config(&root, &mut store, 1); + + // Overwrite the registry with bytes that decode to nothing, and take the + // index with it so no config can answer for the book. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let mut name = heapless::String::::new(); + book_index_file_name(layout_cache_key_for(&store), &mut name); + book.delete_file_in_dir(name.as_str()) + .expect("index deletes"); + let file = book + .open_file_in_dir( + CACHE_CONFIG_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("registry opens"); + file.write(b"garbage!!").expect("registry body"); + drop(file); + drop(book); + + assert_eq!( + files::read_cache_header(&root, KEY), + files::CacheHeader::Unreadable, + "a registry that is there and unusable must not read as no cache at all" + ); +} + +/// Invariant: a `CFG.BIN` write that will not land is reported, not swallowed. +/// +/// The registry is opened create-or-truncate, so a refused write does not leave +/// the previous one behind -- it leaves nothing decodable. An open that shrugged +/// that off would go on to build under a config no registry names. +#[test] +fn a_registry_write_that_is_refused_is_reported() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + build_under_current_config(&root, &mut store, 1); + + // Block the registry the same way the eviction tests block a delete: hold + // it open, which this embedded-sdmmc rev will not reopen for writing. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let held = book + .open_file_in_dir(CACHE_CONFIG_FILE, embedded_sdmmc::Mode::ReadOnly) + .expect("hold the registry open"); + + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let blocked = files::adopt_layout_config(&root, KEY, &store); + assert!( + blocked.registry_write_failed, + "a registry that would not take the write must say so" + ); + assert_eq!( + blocked.eviction_failed, None, + "nothing needed evicting -- this is the write, not the delete" + ); + drop(held); + drop(book); +} + +/// Invariant: a registry left unusable by a refused write does not cost the +/// two-config bound. This is the accumulation the write failure above would +/// otherwise start. +/// +/// A `CFG.BIN` that is there and will not decode used to read as an empty +/// registry, which unregisters every config on the card at once: eviction stops +/// counting them, so the next open adds a third full section set, and the one +/// after that a fourth. Rebuilding the registry from the index files that are +/// really there is what keeps the count honest. +#[test] +fn a_registry_left_unusable_is_rebuilt_from_the_index_files_on_the_card() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let first_key = build_under_current_config(&root, &mut store, 2); + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let second_key = build_under_current_config(&root, &mut store, 2); + + // Exactly what a refused write leaves: the file there, its contents gone. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let file = book + .open_file_in_dir( + CACHE_CONFIG_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("registry opens"); + drop(file); + drop(book); + + // A third config arrives. Both existing configs are still on the card, so + // the bound says one of them goes -- which an empty registry could not know. + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let adoption = files::adopt_layout_config(&root, KEY, &store); + let evicted = adoption + .evicted + .expect("a truncated registry must not hide the configs the card still holds"); + assert!( + evicted == first_key || evicted == second_key, + "eviction must take one of the two configs that were really there, got {evicted:#04x}" + ); + assert!( + !book_index_present(&root, evicted), + "the evicted config's index must actually be gone" + ); + for section in 0..2 { + assert!( + !section_file_present(&root, evicted, section), + "section {section} of the evicted config must go with its index" + ); + } + let survivor = if evicted == first_key { + second_key + } else { + first_key + }; + assert!( + book_index_present(&root, survivor), + "eviction must take one config, not both" + ); +} + +/// Invariant: a legacy purge that cannot finish leaves the marker that brings +/// the next open back for it. +/// +/// `BOOK.BIN` is the only thing that makes a later open pay for a sections +/// listing, so deleting it before the sweep finished would strand the unkeyed +/// section files for good: nothing reads those names and no registry counts +/// them, so no later pass would ever look again. +#[test] +fn a_legacy_sweep_that_is_blocked_keeps_its_retry_marker() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("cache dirs"); + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); + for (dir, name) in [(None, CACHE_BOOK_FILE), (Some(§ions), "S000.BIN")] { + let file = dir + .unwrap_or(&book) + .open_file_in_dir(name, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) + .expect("legacy file"); + file.write(&[0u8; 64]).expect("legacy file body"); + } + + // The legacy section will not delete while it is held open. + let held = sections + .open_file_in_dir("S000.BIN", embedded_sdmmc::Mode::ReadOnly) + .expect("hold the legacy section open"); + + assert!( + files::adopt_layout_config(&root, KEY, &store).purged_legacy, + "the marker was there, so the purge ran" + ); + assert!( + book.open_file_in_dir(CACHE_BOOK_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_ok(), + "the sweep did not finish, so the marker must survive to bring the next open back" + ); + + drop(held); + drop(sections); + drop(book); + + // The block is gone; the retry the marker bought finishes the job. + assert!( + files::adopt_layout_config(&root, KEY, &store).purged_legacy, + "the surviving marker must send the next open back through the purge" + ); + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); + assert!( + sections + .open_file_in_dir("S000.BIN", embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "the retry takes the legacy section the first pass could not" + ); + drop(sections); + assert!( + book.open_file_in_dir(CACHE_BOOK_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "and the marker goes once there is nothing left to come back for" + ); +} + +/// Invariant: no single read fault can make a cache that is plainly on the card +/// read as `Absent`. +/// +/// `Absent` is the one answer that licenses the clear path to delete a +/// directory, against a 28-bit key whose collisions the format admits. The +/// registry is deleted here so the fallback directory listing is the only route +/// to the index -- and a listing that fails must not be indistinguishable from +/// one that completed and found nothing. +#[test] +fn no_read_fault_can_make_a_cache_that_is_there_read_as_absent() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + build_under_current_config(&root, &mut store, 1); + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + book.delete_file_in_dir(CACHE_CONFIG_FILE) + .expect("registry deletes"); + drop(book); + + // Baseline: with no fault armed, the listing finds the index. + let before = disk.reads.get(); + assert!( + matches!( + files::read_cache_header(&root, KEY), + files::CacheHeader::Present(_) + ), + "the unlisted-index scan must find a cache whose registry is gone" + ); + let reads = disk.reads.get() - before; + assert!( + reads > 0, + "the read counter must be moving for this to mean anything" + ); + + // Every read that pass makes, failed one at a time. `Unreadable` is fine -- + // it fails closed. `Absent` is the one answer that would license a delete. + let mut unreadable = 0; + for nth in 0..reads { + disk.fault.fail_read_in.set(Some(nth)); + let header = files::read_cache_header(&root, KEY); + disk.fault.fail_read_in.set(None); + assert_ne!( + header, + files::CacheHeader::Absent, + "read fault {nth} made a cache that is on the card read as deletable" + ); + if header == files::CacheHeader::Unreadable { + unreadable += 1; + } + } + assert!( + unreadable > 0, + "no fault position actually disturbed the read, so this proved nothing" + ); +} + +/// Invariant: clearing a book with two configs resident leaves nothing behind +/// -- and says so. The clear reports success on what the directory actually +/// holds, so a per-config file it did not learn to delete would show up as a +/// failed clear rather than as silent leftovers. +#[test] +fn clearing_a_book_with_two_configs_resident_leaves_nothing_behind() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + build_under_current_config(&root, &mut store, 2); + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + build_under_current_config(&root, &mut store, 2); + write_cover(&root); + + assert!( + files::empty_cache_dir(&root, KEY), + "a clear must reclaim every config's files, not report failure on the ones it missed" + ); + assert_eq!( + files::read_cache_header(&root, KEY), + files::CacheHeader::Absent, + "nothing identifying the book may survive the clear" + ); +} From b6cfb326b6739f843094a7e0ca86989bcb976071 Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Wed, 5 Aug 2026 22:18:35 -0700 Subject: [PATCH 2/8] Fix adoption failure gating, fallible registry reconstruction, and CFG.BIN preservation - Gate new cache build/replay output on successful layout config adoption (adoption.succeeded()), preventing unadopted per-layout files from accumulating on disk. - Make layout_registry_from_index_files fallible and explicitly delete excess non-surviving configurations when >2 indexes are found during reconstruction. - Defer CFG.BIN deletion in empty_cache_dir until per-config indexes and sections are deleted and cleared == true. - Validate v2_toc_label_bounds_ok before computing title_offset in read_v2_book_title. - Remove orphan conflict marker from docs/ARCHITECTURE.md. - Add integration tests for CFG.BIN preservation on failed clear and excess config deletion during reconstruction. --- docs/ARCHITECTURE.md | 1 - fw/src/book_build.rs | 10 ++- proto/src/cache.rs | 8 +++ reader-cache/src/files.rs | 92 +++++++++++++++------------- reader-cache/tests/publish_faults.rs | 76 +++++++++++++++++++++++ 5 files changed, 143 insertions(+), 44 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5b4781a5..ceb22d47 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -416,7 +416,6 @@ only window crossings re-read it. The currently open book sits in a separate scrolled. On boot/refresh, firmware first loads a window from the cached snapshot, then refreshes `/BOOKS` and card-root discovery in a storage command, streaming the fresh catalog out in batches without ever holding it -<<<<<<< HEAD whole. Discovery skips dot-prefixed entries, so the AppleDouble sidecar (`._.epub`) Finder writes beside every file it copies to a FAT card is not catalogued as a phantom, unopenable duplicate. The scan reads long diff --git a/fw/src/book_build.rs b/fw/src/book_build.rs index 688afca8..65f4f220 100644 --- a/fw/src/book_build.rs +++ b/fw/src/book_build.rs @@ -619,12 +619,13 @@ where // retries the eviction rather than losing track of a full section set. let adoption = files::adopt_layout_config(root, cache_key.as_str(), library); esp_println::println!( - "epub: layout config resident={} evicted={:?} evict_failed={:?} legacy_purged={} registry_write_failed={}", + "epub: layout config resident={} evicted={:?} evict_failed={:?} legacy_purged={} registry_write_failed={} dirs_failed={}", adoption.resident, adoption.evicted, adoption.eviction_failed, adoption.purged_legacy, - adoption.registry_write_failed + adoption.registry_write_failed, + adoption.dirs_failed ); esp_println::println!( "epub: stage TryV2BookIndexFast page={}", @@ -659,6 +660,11 @@ where // already been overwritten. scratch.resume = None; } + if !fast_hit && !adoption.succeeded() { + esp_println::println!("epub: adoption failed, refusing to publish new layout cache"); + set_preview_error(library, "ADOPT"); + return BookLoadStatus::Error; + } let replayed = !fast_hit && try_replay_content_cache( root, diff --git a/proto/src/cache.rs b/proto/src/cache.rs index 696febda..17968a59 100644 --- a/proto/src/cache.rs +++ b/proto/src/cache.rs @@ -775,6 +775,14 @@ impl LayoutConfigRegistry { &self.keys[..self.count] } + pub fn len(&self) -> usize { + self.count + } + + pub fn is_empty(&self) -> bool { + self.count == 0 + } + /// The config the book was last read under, if it has ever been built. pub fn most_recent(&self) -> Option { self.keys().first().copied() diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index b136ff6d..cc6aee59 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -856,6 +856,7 @@ where let header = decode_book_v2_header(&header_bytes).ok()?; if header.source_hash != source_identity.0 || header.source_size != source_identity.1 + || !v2_toc_label_bounds_ok(&header) || header.title_text_bytes == 0 || header.title_text_bytes as usize > 64 { @@ -1101,7 +1102,6 @@ where CACHE_TOC_FILE, CACHE_COVER_FILE, CACHE_CONTENT_FILE, - CACHE_CONFIG_FILE, ] { if upload_store::remove_file_reclaiming_clusters(&book, name) == upload_store::RemoveStatus::Failed @@ -1110,10 +1110,11 @@ where } } // The per-config indexes, by listing rather than by name: the - // registry that would name them has just been deleted, and it was - // never the authority on what is on the card anyway. A key past the - // listing budget survives to fail `book_dir_is_reclaimed` below, - // which is what stops this from reporting a clear it did not finish. + // registry that names them stays until they are gone, so an + // interrupted clear leaves surviving indexes accounted for. A key + // past the listing budget survives to fail `book_dir_is_reclaimed` + // below, which is what stops this from reporting a clear it did not + // finish. match book_index_names_unlisted_by(&book, &LayoutConfigRegistry::new()) { Some(names) => { for name in &names { @@ -1144,6 +1145,11 @@ where // leaves an empty directory, not cache data, so it does not make // the clear a failure. let _ = book.delete_file_in_dir(CACHE_SECTIONS_DIR); + if upload_store::remove_file_reclaiming_clusters(&book, CACHE_CONFIG_FILE) + == upload_store::RemoveStatus::Failed + { + cleared = false; + } } // Everything above worked from a list of names. This is the part that // does not: it asks the directory what is actually left, which is the @@ -1919,7 +1925,6 @@ where } } -/// What adopting a layout config found waiting for it. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct LayoutConfigAdoption { /// The registry already listed this config, so a cache built under it @@ -1942,12 +1947,17 @@ pub struct LayoutConfigAdoption { /// firmware that wrote them. pub purged_legacy: bool, /// The registry write did not land, so `CFG.BIN` no longer describes which - /// configs are on the card. The open continues — see - /// [`adopt_layout_config`] for why refusing it would be worse — and the - /// next one repairs the registry by listing the index files that are - /// really there, which is what holds the bound. Reported so the log names - /// the card that is failing small writes. + /// configs are on the card. Reported so the log names the card that is + /// failing small writes. pub registry_write_failed: bool, + /// Directory access or creation failed on the way in. + pub dirs_failed: bool, +} + +impl LayoutConfigAdoption { + pub fn succeeded(&self) -> bool { + !self.dirs_failed && self.eviction_failed.is_none() && !self.registry_write_failed + } } /// Claim the layout config `library` is reading under as this book's most @@ -1988,15 +1998,18 @@ where eviction_failed: None, purged_legacy: false, registry_write_failed: false, + dirs_failed: false, }; // The tree has to exist before the registry can be written into it, and // this is the earliest point in an open that knows the book's key. A // failure here fails the build too, so there is nothing else to do. if ensure_v2_cache_dirs(root, key).is_err() { cache_log!("cache: adopt config ensure dirs failed key={}", key); + adoption.dirs_failed = true; return adoption; } let Some(book) = open_v2_book_dir(root, key) else { + adoption.dirs_failed = true; return adoption; }; adoption.purged_legacy = purge_legacy_artifacts_in(&book); @@ -2013,16 +2026,16 @@ where // whatever the listing gave. The config being adopted is promoted to the // front below either way, and the worst a wrong order does is evict the // less useful of two survivors. - // - // A registry that is not there at all is left to start empty: that is a - // book being opened for the first time, which is the common case and must - // not pay for a directory listing. A `CFG.BIN` deleted outright while index - // files remain is the one gap left — the clear still sweeps by listing, and - // `read_cache_header`'s fallback still finds the book. let mut registry = match read_layout_registry_in(&book) { Some(registry) => registry, - None if registry_present(&book) => layout_registry_from_index_files(&book), - None => LayoutConfigRegistry::new(), + None => match layout_registry_from_index_files(&book) { + Some(registry) if !registry.is_empty() || registry_present(&book) => registry, + Some(_) => LayoutConfigRegistry::new(), + None => { + adoption.registry_write_failed = true; + return adoption; + } + }, }; adoption.resident = registry.contains(layout_key); let evicted = registry.promote(layout_key); @@ -2058,25 +2071,22 @@ where key, layout_key ); - // Reported rather than fatal. The build below still writes under this - // config and its index is still found — by the next open under the same - // config once the registry is repaired, and by the unlisted-index scan - // meanwhile — so refusing the open here would cost a readable book to - // buy nothing. What the failure does cost is the registry's account of - // the card, and the seeding above is what gets that back. adoption.registry_write_failed = true; } adoption } /// Rebuild a registry from the per-config index files a book directory actually -/// holds, for an open whose `CFG.BIN` is there and unusable. +/// holds. /// /// Ordering is lost — a listing cannot say which config was read last — so this /// only restores the *count*, which is the part eviction needs to hold the -/// two-config bound. A card holding more sets than the bound (which is how it -/// got here) leaves the extras unnamed for now; the eviction the caller runs -/// next brings the count down, and later opens converge. +/// two-config bound. +/// +/// When more than two configurations exist, any excess key evicted during +/// promotion is explicitly deleted from the card before returning. If directory +/// enumeration or an excess file deletion fails, `None` is returned to prevent +/// an incomplete or corrupt registry from being committed. fn layout_registry_from_index_files< D, T, @@ -2085,27 +2095,27 @@ fn layout_registry_from_index_files< const MAX_VOLUMES: usize, >( book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, -) -> LayoutConfigRegistry +) -> Option where D: embedded_sdmmc::BlockDevice, T: TimeSource, { let mut registry = LayoutConfigRegistry::new(); - // Every keyed index is "unlisted" by an empty registry, so this is the same - // bounded listing `read_cache_header` falls back to. A listing that would - // not run leaves the registry empty, which is where it started. - let Some(names) = book_index_names_unlisted_by(book, ®istry) else { - return registry; - }; + let names = book_index_names_unlisted_by(book, ®istry)?; for name in &names { if let Some(layout_key) = layout_key_of_book_index_file_name(name.as_str()) { - // Seeding, not evicting: nothing is deleted here, so a key pushed - // out of the last slot is simply one this pass could not account - // for. - let _ = registry.promote(layout_key); + if let Some(evicted) = registry.promote(layout_key) { + if !delete_layout_config_artifacts_in(book, evicted) { + cache_log!( + "cache: reconstruct registry failed to delete excess key {}", + evicted + ); + return None; + } + } } } - registry + Some(registry) } /// Delete one layout config's cache files: its index and every section file diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index b7510522..760c7100 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -1804,3 +1804,79 @@ fn clearing_a_book_with_two_configs_resident_leaves_nothing_behind() { "nothing identifying the book may survive the clear" ); } + +/// Invariant: CFG.BIN is preserved if a full-cache clear fails partway through, +/// ensuring surviving layout configurations remain tracked for LRU eviction. +#[test] +fn cfg_bin_is_preserved_if_cache_clear_fails() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + build_under_current_config(&root, &mut store, 2); + + // Block a section file from being deleted during clear. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let sections = book.open_dir("SECTIONS").expect("sections dir"); + let section_file = sections + .open_file_in_dir("S00000.BIN", embedded_sdmmc::Mode::ReadOnly) + .expect("hold section file open"); + + assert!( + !files::empty_cache_dir(&root, KEY), + "a clear with a blocked section file must return false" + ); + + // CFG.BIN must still be present in the book directory. + assert!( + book.open_file_in_dir( + proto::cache::CACHE_CONFIG_FILE, + embedded_sdmmc::Mode::ReadOnly + ) + .is_ok(), + "CFG.BIN must survive a failed clear so LRU tracking is not lost" + ); + + drop(section_file); + drop(sections); + drop(book); +} + +/// Invariant: reconstructing a registry from more than two indexes explicitly +/// deletes excess non-surviving configurations from the card before committing. +#[test] +fn reconstruct_registry_removes_excess_configs_and_succeeds() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let _k1 = build_under_current_config(&root, &mut store, 1); + + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let _k2 = build_under_current_config(&root, &mut store, 1); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let _k3 = build_under_current_config(&root, &mut store, 1); + + // Now corrupt CFG.BIN so adoption must rebuild the registry from index files. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let cfg_file = book + .open_file_in_dir( + proto::cache::CACHE_CONFIG_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("open CFG.BIN"); + drop(cfg_file); + drop(book); + + // Rebuilding will find 3 index files on disk (_k1, _k2, _k3). It must evict & delete the 3rd one found. + let adoption = files::adopt_layout_config(&root, KEY, &store); + assert!( + adoption.succeeded(), + "reconstruction from index files with excess configs must succeed" + ); +} From 254be2e4fd1e1d6ec8563e63c962e1832c9dcc24 Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Wed, 5 Aug 2026 22:35:10 -0700 Subject: [PATCH 3/8] Handle reconstruction inventory overflow and sections-first deletion --- fw/src/book_build.rs | 8 +- reader-cache/src/files.rs | 59 +++++------ reader-cache/tests/publish_faults.rs | 150 +++++++++++++++++++++++++-- 3 files changed, 174 insertions(+), 43 deletions(-) diff --git a/fw/src/book_build.rs b/fw/src/book_build.rs index 65f4f220..268fae6a 100644 --- a/fw/src/book_build.rs +++ b/fw/src/book_build.rs @@ -613,10 +613,10 @@ where // recently used config's files have to be gone before a build adds a // third set. // - // An eviction that would not go through leaves this config unadopted: the - // build below still runs and writes under it, but the registry keeps - // naming the config whose files are still on the card, so the next open - // retries the eviction rather than losing track of a full section set. + // An eviction or registry write that does not succeed leaves this config + // unadopted. An existing cache hit can still be read, but any non-fast-hit + // build or replay is refused below so unadopted layout files are never + // published to disk. let adoption = files::adopt_layout_config(root, cache_key.as_str(), library); esp_println::println!( "epub: layout config resident={} evicted={:?} evict_failed={:?} legacy_purged={} registry_write_failed={} dirs_failed={}", diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index cc6aee59..aa4085ed 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -2101,34 +2101,35 @@ where T: TimeSource, { let mut registry = LayoutConfigRegistry::new(); - let names = book_index_names_unlisted_by(book, ®istry)?; - for name in &names { - if let Some(layout_key) = layout_key_of_book_index_file_name(name.as_str()) { - if let Some(evicted) = registry.promote(layout_key) { - if !delete_layout_config_artifacts_in(book, evicted) { - cache_log!( - "cache: reconstruct registry failed to delete excess key {}", - evicted - ); - return None; + for _ in 0..64 { + let names = book_index_names_unlisted_by(book, ®istry)?; + if names.is_empty() { + return Some(registry); + } + for name in &names { + if let Some(layout_key) = layout_key_of_book_index_file_name(name.as_str()) { + if let Some(evicted) = registry.promote(layout_key) { + if !delete_layout_config_artifacts_in(book, evicted) { + cache_log!( + "cache: reconstruct registry failed to delete excess key {}", + evicted + ); + return None; + } } } } } - Some(registry) + None } -/// Delete one layout config's cache files: its index and every section file -/// named for it. The other configs' files, and everything -/// settings-independent (TOC.BIN, COVER.BIN, CONT.BIN, the position), stay. +/// Delete one layout config's cache files: its section files and its index. +/// The other configs' files, and everything settings-independent (TOC.BIN, +/// COVER.BIN, CONT.BIN, the position), stay. /// -/// Returns whether every one of that config's files is now gone — deleted -/// here or already absent. This is the answer the caller's registry write -/// hangs on, so anything it cannot account for reads as false: a delete that -/// refused, a `SECTIONS/` that would not open, a listing that would not -/// finish. The index goes first, so a sweep that stops partway leaves an -/// unreadable set (stray sections, no index) rather than an index promising -/// sections that are gone. +/// Section files go first and the index last. Keeping the index until all of +/// its section files are gone ensures that an interrupted deletion leaves the +/// index on disk as the durable marker for reconstruction on the next open. fn delete_layout_config_artifacts_in< D, T, @@ -2143,14 +2144,7 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { - let mut name = String::::new(); - book_index_file_name(layout_key, &mut name); - if upload_store::remove_file_reclaiming_clusters(book, name.as_str()) - == upload_store::RemoveStatus::Failed - { - return false; - } - match book.open_dir(CACHE_SECTIONS_DIR) { + let sections_ok = match book.open_dir(CACHE_SECTIONS_DIR) { Ok(sections) => sweep_section_files(§ions, |name| { layout_key_of_section_file_name(name) == Some(layout_key) }), @@ -2158,7 +2152,14 @@ where Err(embedded_sdmmc::Error::NotFound) => true, // A directory that would not open may still hold the config's files. Err(_) => false, + }; + if !sections_ok { + return false; } + let mut name = String::::new(); + book_index_file_name(layout_key, &mut name); + upload_store::remove_file_reclaiming_clusters(book, name.as_str()) + != upload_store::RemoveStatus::Failed } /// Delete the artifacts of the single-config scheme this replaced: the diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index 760c7100..913d233e 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -1282,10 +1282,6 @@ fn an_eviction_that_cannot_delete_keeps_the_evicted_config_registered() { "the refused delete leaves the index on the card" ); for section in 0..2 { - assert!( - section_file_present(&root, first_key, section), - "the index refusal stops the sweep, so section {section} stays too" - ); assert!( section_file_present(&root, second_key, section), "the surviving config is untouched either way" @@ -1354,8 +1350,8 @@ fn an_eviction_that_cannot_delete_a_section_file_reports_no_eviction() { drop(sections); drop(book); assert!( - !book_index_present(&root, first_key), - "the index goes first, and that part did succeed" + book_index_present(&root, first_key), + "sections go first, so the index remains when a section delete fails" ); assert!( section_file_present(&root, first_key, 1), @@ -1366,7 +1362,7 @@ fn an_eviction_that_cannot_delete_a_section_file_reports_no_eviction() { assert_eq!( retry.evicted, Some(first_key), - "a config whose index is already gone is still the one owed a sweep" + "a config whose sections failed to delete is still the one owed a sweep" ); assert!( !section_file_present(&root, first_key, 1), @@ -1843,6 +1839,19 @@ fn cfg_bin_is_preserved_if_cache_clear_fails() { drop(book); } +/// Helper to build an index and section files without adopting into the registry. +fn build_raw_config_index(root: &Dir<'_>, store: &mut ReaderStore, sections: usize) -> u8 { + let layout_key = layout_cache_key_for(store); + files::ensure_v2_cache_dirs(root, KEY).expect("dirs"); + let records = build_book(root, store, sections); + let pages = total_pages(&records); + assert!( + files::write_v2_book_index(root, KEY, IDENTITY, pages, &records, store, false, 0), + "the index for config {layout_key:#04x} should write" + ); + layout_key +} + /// Invariant: reconstructing a registry from more than two indexes explicitly /// deletes excess non-surviving configurations from the card before committing. #[test] @@ -1852,15 +1861,15 @@ fn reconstruct_registry_removes_excess_configs_and_succeeds() { let root = open_root(&mgr); let mut store = new_store(); - let _k1 = build_under_current_config(&root, &mut store, 1); + let _k1 = build_raw_config_index(&root, &mut store, 1); let (settings, portrait) = landscape_of(&store); store.set_layout(settings, portrait); - let _k2 = build_under_current_config(&root, &mut store, 1); + let _k2 = build_raw_config_index(&root, &mut store, 1); let (settings, portrait) = larger_of(&store); store.set_layout(settings, portrait); - let _k3 = build_under_current_config(&root, &mut store, 1); + let _k3 = build_raw_config_index(&root, &mut store, 1); // Now corrupt CFG.BIN so adoption must rebuild the registry from index files. let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); @@ -1880,3 +1889,124 @@ fn reconstruct_registry_removes_excess_configs_and_succeeds() { "reconstruction from index files with excess configs must succeed" ); } + +/// Invariant: registry reconstruction handles an overflowing inventory containing +/// five keyed indexes across multiple listing passes, deleting all excess configs +/// and committing a 2-slot registry. +#[test] +fn reconstruct_registry_handles_overflowing_inventory_with_five_configs() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let k1 = build_raw_config_index(&root, &mut store, 1); + + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let k2 = build_raw_config_index(&root, &mut store, 1); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let k3 = build_raw_config_index(&root, &mut store, 1); + + let mut settings = store.type_settings(); + settings.size = display::font::FontSize::Small; + store.set_layout(settings, store.portrait()); + let k4 = build_raw_config_index(&root, &mut store, 1); + + let mut settings = store.type_settings(); + settings.weight = display::font::FontWeight::Heavy; + store.set_layout(settings, store.portrait()); + let k5 = build_raw_config_index(&root, &mut store, 1); + + // Corrupt CFG.BIN so adoption must reconstruct from 5 index files on disk. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let cfg_file = book + .open_file_in_dir( + proto::cache::CACHE_CONFIG_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("open CFG.BIN"); + drop(cfg_file); + drop(book); + + let adoption = files::adopt_layout_config(&root, KEY, &store); + assert!( + adoption.succeeded(), + "reconstructing from 5 index files must succeed by looping passes" + ); + + // Verify exactly 2 of the 5 configs remain present on disk. + let present_count = [k1, k2, k3, k4, k5] + .iter() + .filter(|&&k| book_index_present(&root, k)) + .count(); + assert_eq!( + present_count, 2, + "reconstruction must leave exactly two config indexes on disk" + ); +} + +/// Invariant: section files of an excess config are deleted before its index. +/// An interrupted section deletion leaves the index intact as a durable marker +/// so the next open retries the cleanup before committing CFG.BIN. +#[test] +fn reconstruct_retries_cleanup_when_excess_section_delete_fails() { + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + let k1 = build_raw_config_index(&root, &mut store, 1); + + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let _k2 = build_raw_config_index(&root, &mut store, 1); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let _k3 = build_raw_config_index(&root, &mut store, 1); + + // Corrupt CFG.BIN so adoption must reconstruct. + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let cfg_file = book + .open_file_in_dir( + proto::cache::CACHE_CONFIG_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("open CFG.BIN"); + drop(cfg_file); + + // Hold a section file of k1 open so its section deletion fails. + let sections = book.open_dir("SECTIONS").expect("sections dir"); + let mut sname = heapless::String::::new(); + section_file_name(k1, 0, &mut sname); + let held_section = sections + .open_file_in_dir(sname.as_str(), embedded_sdmmc::Mode::ReadOnly) + .expect("hold k1 section file open"); + + let blocked = files::adopt_layout_config(&root, KEY, &store); + assert!( + !blocked.succeeded(), + "adoption must fail when excess section deletion fails" + ); + assert!( + book_index_present(&root, k1), + "BK.BIN must survive when section deletion fails so it can be retried" + ); + + drop(held_section); + drop(sections); + drop(book); + + let retry = files::adopt_layout_config(&root, KEY, &store); + assert!( + retry.succeeded(), + "adoption must succeed on retry once section block is cleared" + ); + assert!( + !book_index_present(&root, k1), + "BK.BIN must be removed after successful section deletion" + ); +} From e5c0c98efb46bdd05ad91a9ea2aac64ad1604010 Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Wed, 5 Aug 2026 22:54:05 -0700 Subject: [PATCH 4/8] Enforce single source owner during layout config adoption --- fw/src/book_build.rs | 2 +- reader-cache/src/files.rs | 130 ++++++++++++++++++++++++++- reader-cache/tests/publish_faults.rs | 106 +++++++++++++++++----- 3 files changed, 212 insertions(+), 26 deletions(-) diff --git a/fw/src/book_build.rs b/fw/src/book_build.rs index 268fae6a..6b3c9e07 100644 --- a/fw/src/book_build.rs +++ b/fw/src/book_build.rs @@ -617,7 +617,7 @@ where // unadopted. An existing cache hit can still be read, but any non-fast-hit // build or replay is refused below so unadopted layout files are never // published to disk. - let adoption = files::adopt_layout_config(root, cache_key.as_str(), library); + let adoption = files::adopt_layout_config(root, cache_key.as_str(), source_identity, library); esp_println::println!( "epub: layout config resident={} evicted={:?} evict_failed={:?} legacy_purged={} registry_write_failed={} dirs_failed={}", adoption.resident, diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index aa4085ed..03b2c86b 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -1058,9 +1058,10 @@ const SHORT_NAME_BYTES: usize = 12; /// forever. `SECTIONS/` is enumerated instead, so what is on the card decides /// what gets deleted. /// -/// BOOK.BIN goes first, deliberately. It is the cache's liveness marker, so an -/// interrupted delete leaves a cache that reads as absent and gets rebuilt, -/// rather than a header advertising sections that are no longer there. +/// Per-config index files (`BK*.BIN`) and section files are removed first, and +/// `CFG.BIN` goes last once all artifacts are cleared. Keeping `CFG.BIN` until +/// the end ensures an interrupted clear preserves the registry so surviving +/// per-config files remain accounted for on subsequent opens. /// /// The global reading position in XTEINK/STATE.BIN is never touched. pub fn empty_cache_dir< @@ -1985,6 +1986,7 @@ pub fn adopt_layout_config< >( root: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, key: &str, + source_identity: (u32, u32), library: &ReaderStore, ) -> LayoutConfigAdoption where @@ -2012,7 +2014,16 @@ where adoption.dirs_failed = true; return adoption; }; - adoption.purged_legacy = purge_legacy_artifacts_in(&book); + if !book_dir_belongs_to_source_identity(&book, source_identity) { + cache_log!("cache: adopt config identity mismatch key={}", key); + if !purge_colliding_cache_artifacts_in(&book) { + adoption.dirs_failed = true; + return adoption; + } + adoption.purged_legacy = true; + } else { + adoption.purged_legacy = purge_legacy_artifacts_in(&book); + } // A registry file that is there and will not decode gets rebuilt from the // index files that really are on the card, rather than started over from // empty. Starting empty is what loses the bound: the configs already there @@ -2162,6 +2173,117 @@ where != upload_store::RemoveStatus::Failed } +/// Check whether every readable index file or TOC file in a book cache directory +/// matches the expected source identity. Returns false if any readable header +/// identifies a different source book (a 28-bit key collision). +fn book_dir_belongs_to_source_identity< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, + source_identity: (u32, u32), +) -> bool +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let stored = read_layout_registry_in(book); + let registry = stored.unwrap_or_default(); + let mut name = String::::new(); + for layout_key in registry.keys() { + book_index_file_name(*layout_key, &mut name); + if let Some(Some(header)) = read_book_index_header(book, name.as_str()) { + if header.source_hash != source_identity.0 || header.source_size != source_identity.1 { + return false; + } + } + } + if let Some(unlisted_names) = book_index_names_unlisted_by(book, ®istry) { + for unlisted in unlisted_names { + if let Some(Some(header)) = read_book_index_header(book, unlisted.as_str()) { + if header.source_hash != source_identity.0 + || header.source_size != source_identity.1 + { + return false; + } + } + } + } + if let Ok(file) = book.open_file_in_dir(CACHE_TOC_FILE, Mode::ReadOnly) { + let mut header_bytes = [0u8; TOC_FILE_HEADER_BYTES]; + if read_exact_file(&file, &mut header_bytes).is_ok() { + if let Ok(header) = decode_toc_file_header(&header_bytes) { + if header.source_hash != source_identity.0 + || header.source_size != source_identity.1 + { + return false; + } + } + } + } + true +} + +/// Remove all cache files (indexes, sections, TOC/COVER/CONT, CFG.BIN) in a +/// directory that was occupied by a colliding book identity. +fn purge_colliding_cache_artifacts_in< + D, + T, + const MAX_DIRS: usize, + const MAX_FILES: usize, + const MAX_VOLUMES: usize, +>( + book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, +) -> bool +where + D: embedded_sdmmc::BlockDevice, + T: TimeSource, +{ + let mut cleared = true; + for name in [ + CACHE_BOOK_FILE, + CACHE_TOC_FILE, + CACHE_COVER_FILE, + CACHE_CONTENT_FILE, + ] { + if upload_store::remove_file_reclaiming_clusters(book, name) + == upload_store::RemoveStatus::Failed + { + cleared = false; + } + } + match book_index_names_unlisted_by(book, &LayoutConfigRegistry::new()) { + Some(names) => { + for name in &names { + if upload_store::remove_file_reclaiming_clusters(book, name.as_str()) + == upload_store::RemoveStatus::Failed + { + cleared = false; + } + } + } + None => cleared = false, + } + match book.open_dir(CACHE_SECTIONS_DIR) { + Ok(sections) => { + if !empty_sections_dir(§ions) { + cleared = false; + } + } + Err(embedded_sdmmc::Error::NotFound) => {} + Err(_) => cleared = false, + } + if upload_store::remove_file_reclaiming_clusters(book, CACHE_CONFIG_FILE) + == upload_store::RemoveStatus::Failed + { + cleared = false; + } + cleared +} + /// Delete the artifacts of the single-config scheme this replaced: the /// unkeyed `BOOK.BIN` and the unkeyed `S.BIN` section files. /// diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index 913d233e..4861341c 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -876,7 +876,7 @@ fn the_prune_leaves_the_other_config_alone() { shrinking_key, keeper_key, "the flip must land on a different config for this to test anything" ); - files::adopt_layout_config(&root, KEY, &store); + files::adopt_layout_config(&root, KEY, IDENTITY, &store); let wide = build_book(&root, &mut store, 5); let narrow = &wide[..2]; store.begin_book_load(); @@ -1094,7 +1094,7 @@ fn book_index_present(root: &Dir<'_>, layout_key: u8) -> bool { /// write its index. Returns the config's cache key. fn build_under_current_config(root: &Dir<'_>, store: &mut ReaderStore, sections: usize) -> u8 { let layout_key = layout_cache_key_for(store); - files::adopt_layout_config(root, KEY, store); + files::adopt_layout_config(root, KEY, IDENTITY, store); let records = build_book(root, store, sections); let pages = total_pages(&records); assert!( @@ -1146,7 +1146,7 @@ fn a_second_layout_config_does_not_overwrite_the_first() { // without any rebuild. let (settings, portrait) = (store.type_settings(), !store.portrait()); store.set_layout(settings, portrait); - let adoption = files::adopt_layout_config(&root, KEY, &store); + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert!( adoption.resident, "the config just flipped away from must read as already built" @@ -1188,7 +1188,7 @@ fn a_third_layout_config_evicts_the_least_recently_used_one() { let (settings, portrait) = larger_of(&store); store.set_layout(settings, portrait); let third_key = layout_cache_key_for(&store); - let adoption = files::adopt_layout_config(&root, KEY, &store); + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert_eq!( adoption.evicted, Some(first_key), @@ -1264,7 +1264,7 @@ fn an_eviction_that_cannot_delete_keeps_the_evicted_config_registered() { let (settings, portrait) = larger_of(&store); store.set_layout(settings, portrait); - let blocked = files::adopt_layout_config(&root, KEY, &store); + let blocked = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert_eq!( blocked.evicted, None, "nothing was deleted, so nothing may be reported evicted" @@ -1291,7 +1291,7 @@ fn an_eviction_that_cannot_delete_keeps_the_evicted_config_registered() { // The whole point of leaving the registry alone: the config is still // named, so the next open under the third config evicts it rather than // walking past files nothing counts. - let retry = files::adopt_layout_config(&root, KEY, &store); + let retry = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert_eq!( retry.evicted, Some(first_key), @@ -1339,7 +1339,7 @@ fn an_eviction_that_cannot_delete_a_section_file_reports_no_eviction() { let (settings, portrait) = larger_of(&store); store.set_layout(settings, portrait); - let blocked = files::adopt_layout_config(&root, KEY, &store); + let blocked = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert_eq!( blocked.evicted, None, "a sweep that left a file behind is not an eviction" @@ -1358,7 +1358,7 @@ fn an_eviction_that_cannot_delete_a_section_file_reports_no_eviction() { "the section that refused to go is still on the card" ); - let retry = files::adopt_layout_config(&root, KEY, &store); + let retry = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert_eq!( retry.evicted, Some(first_key), @@ -1388,13 +1388,13 @@ fn re_reading_a_config_keeps_it_off_the_eviction_block() { // Go back to the first config without building: it becomes most recent. let (settings, portrait) = (store.type_settings(), !store.portrait()); store.set_layout(settings, portrait); - assert!(files::adopt_layout_config(&root, KEY, &store).resident); + assert!(files::adopt_layout_config(&root, KEY, IDENTITY, &store).resident); // A third config now takes the *other* one's slot. let (settings, portrait) = larger_of(&store); store.set_layout(settings, portrait); assert_eq!( - files::adopt_layout_config(&root, KEY, &store).evicted, + files::adopt_layout_config(&root, KEY, IDENTITY, &store).evicted, Some(second_key), "eviction must take the config that has not been read since, not the one just re-read" ); @@ -1436,7 +1436,7 @@ fn the_first_open_after_the_single_config_scheme_purges_its_files() { write_cover(&root); drop(book); - let adoption = files::adopt_layout_config(&root, KEY, &store); + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert!( adoption.purged_legacy, "an unkeyed BOOK.BIN must be recognized as the previous scheme's" @@ -1469,7 +1469,7 @@ fn the_first_open_after_the_single_config_scheme_purges_its_files() { // A second open has nothing left to find, so the pass costs one failed // open rather than a sections listing. - assert!(!files::adopt_layout_config(&root, KEY, &store).purged_legacy); + assert!(!files::adopt_layout_config(&root, KEY, IDENTITY, &store).purged_legacy); } /// Invariant: a cache whose registry was lost still identifies its book. @@ -1572,7 +1572,7 @@ fn a_registry_write_that_is_refused_is_reported() { let (settings, portrait) = landscape_of(&store); store.set_layout(settings, portrait); - let blocked = files::adopt_layout_config(&root, KEY, &store); + let blocked = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert!( blocked.registry_write_failed, "a registry that would not take the write must say so" @@ -1621,7 +1621,7 @@ fn a_registry_left_unusable_is_rebuilt_from_the_index_files_on_the_card() { // the bound says one of them goes -- which an empty registry could not know. let (settings, portrait) = larger_of(&store); store.set_layout(settings, portrait); - let adoption = files::adopt_layout_config(&root, KEY, &store); + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY, &store); let evicted = adoption .evicted .expect("a truncated registry must not hide the configs the card still holds"); @@ -1681,8 +1681,8 @@ fn a_legacy_sweep_that_is_blocked_keeps_its_retry_marker() { .expect("hold the legacy section open"); assert!( - files::adopt_layout_config(&root, KEY, &store).purged_legacy, - "the marker was there, so the purge ran" + files::adopt_layout_config(&root, KEY, IDENTITY, &store).purged_legacy, + "the purge marker must survive a single-config sweep" ); assert!( book.open_file_in_dir(CACHE_BOOK_FILE, embedded_sdmmc::Mode::ReadOnly) @@ -1696,7 +1696,7 @@ fn a_legacy_sweep_that_is_blocked_keeps_its_retry_marker() { // The block is gone; the retry the marker bought finishes the job. assert!( - files::adopt_layout_config(&root, KEY, &store).purged_legacy, + files::adopt_layout_config(&root, KEY, IDENTITY, &store).purged_legacy, "the surviving marker must send the next open back through the purge" ); let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); @@ -1883,7 +1883,7 @@ fn reconstruct_registry_removes_excess_configs_and_succeeds() { drop(book); // Rebuilding will find 3 index files on disk (_k1, _k2, _k3). It must evict & delete the 3rd one found. - let adoption = files::adopt_layout_config(&root, KEY, &store); + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert!( adoption.succeeded(), "reconstruction from index files with excess configs must succeed" @@ -1931,7 +1931,7 @@ fn reconstruct_registry_handles_overflowing_inventory_with_five_configs() { drop(cfg_file); drop(book); - let adoption = files::adopt_layout_config(&root, KEY, &store); + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert!( adoption.succeeded(), "reconstructing from 5 index files must succeed by looping passes" @@ -1986,7 +1986,7 @@ fn reconstruct_retries_cleanup_when_excess_section_delete_fails() { .open_file_in_dir(sname.as_str(), embedded_sdmmc::Mode::ReadOnly) .expect("hold k1 section file open"); - let blocked = files::adopt_layout_config(&root, KEY, &store); + let blocked = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert!( !blocked.succeeded(), "adoption must fail when excess section deletion fails" @@ -2000,7 +2000,7 @@ fn reconstruct_retries_cleanup_when_excess_section_delete_fails() { drop(sections); drop(book); - let retry = files::adopt_layout_config(&root, KEY, &store); + let retry = files::adopt_layout_config(&root, KEY, IDENTITY, &store); assert!( retry.succeeded(), "adoption must succeed on retry once section block is cleared" @@ -2010,3 +2010,67 @@ fn reconstruct_retries_cleanup_when_excess_section_delete_fails() { "BK.BIN must be removed after successful section deletion" ); } + +/// Invariant: adopting a cache key occupied by a different source identity +/// (a 28-bit hash collision) clears the old owner's cache files before adopting +/// the new book under its own layout configuration. +#[test] +fn adopting_colliding_key_clears_old_owner_cache_and_adopts_new_owner() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + // Book A builds under Layout 1 (default store layout) + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records_a = build_book(&root, &mut store, 1); + let pages_a = total_pages(&records_a); + let k1 = layout_cache_key_for(&store); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages_a, &records_a, &store, false, 0 + )); + + assert!(book_index_present(&root, k1)); + let files::CacheHeader::Present(header_a) = files::read_cache_header(&root, KEY) else { + panic!("expected header A"); + }; + assert_eq!((header_a.source_hash, header_a.source_size), IDENTITY_A); + + // Book B opens under a colliding key with Layout 2 (landscape) + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let k2 = layout_cache_key_for(&store); + assert_ne!(k1, k2, "test requires two different layout keys"); + + let adoption_b = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + adoption_b.purged_legacy, + "adopting a colliding key must report legacy/colliding purge" + ); + + let records_b = build_book(&root, &mut store, 1); + let pages_b = total_pages(&records_b); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_B, pages_b, &records_b, &store, false, 0 + )); + + // Book A's index is gone; Book B's index is present. + assert!( + !book_index_present(&root, k1), + "Book A's index must be purged when Book B adopts the colliding key" + ); + assert!( + book_index_present(&root, k2), + "Book B's index must be present after build" + ); + + // read_cache_header now reports Book B's identity. + let files::CacheHeader::Present(header_b) = files::read_cache_header(&root, KEY) else { + panic!("expected header B"); + }; + assert_eq!((header_b.source_hash, header_b.source_size), IDENTITY_B); +} From 828bbe15e2b388c91ca46ad45246a8dd02b61826 Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Wed, 5 Aug 2026 23:08:00 -0700 Subject: [PATCH 5/8] Fail closed on unreadable ownership and preserve markers during collision purge --- proto/src/cache.rs | 11 ++ reader-cache/src/files.rs | 175 +++++++++++++++------- reader-cache/tests/publish_faults.rs | 212 +++++++++++++++++++++++++++ 3 files changed, 342 insertions(+), 56 deletions(-) diff --git a/proto/src/cache.rs b/proto/src/cache.rs index 17968a59..a046ff91 100644 --- a/proto/src/cache.rs +++ b/proto/src/cache.rs @@ -2241,6 +2241,17 @@ mod tests { assert_eq!(registry.keys(), &[17]); registry.forget(4); assert_eq!(registry.keys(), &[17]); + + // Forgetting key 0 when absent, or forgetting on an empty registry, + // searches only active slots and has no effect. + registry.forget(0); + assert_eq!(registry.keys(), &[17]); + + let mut empty = LayoutConfigRegistry::new(); + empty.forget(0); + assert_eq!(empty.keys(), &[]); + empty.forget(4); + assert_eq!(empty.keys(), &[]); } #[test] diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index 03b2c86b..661ba5aa 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -2014,15 +2014,23 @@ where adoption.dirs_failed = true; return adoption; }; - if !book_dir_belongs_to_source_identity(&book, source_identity) { - cache_log!("cache: adopt config identity mismatch key={}", key); - if !purge_colliding_cache_artifacts_in(&book) { + match book_dir_ownership(&book, source_identity) { + BookDirOwnership::Match => { + adoption.purged_legacy = purge_legacy_artifacts_in(&book); + } + BookDirOwnership::Mismatch => { + cache_log!("cache: adopt config identity mismatch key={}", key); + if !purge_colliding_cache_artifacts_in(&book) { + adoption.dirs_failed = true; + return adoption; + } + adoption.purged_legacy = true; + } + BookDirOwnership::Unreadable => { + cache_log!("cache: adopt config identity unreadable key={}", key); adoption.dirs_failed = true; return adoption; } - adoption.purged_legacy = true; - } else { - adoption.purged_legacy = purge_legacy_artifacts_in(&book); } // A registry file that is there and will not decode gets rebuilt from the // index files that really are on the card, rather than started over from @@ -2173,10 +2181,19 @@ where != upload_store::RemoveStatus::Failed } -/// Check whether every readable index file or TOC file in a book cache directory -/// matches the expected source identity. Returns false if any readable header -/// identifies a different source book (a 28-bit key collision). -fn book_dir_belongs_to_source_identity< +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum BookDirOwnership { + Match, + Mismatch, + Unreadable, +} + +/// Check whether existing headers in a book cache directory match the expected +/// source identity. Returns `Mismatch` if any readable header identifies a +/// different source book (a 28-bit key collision), `Unreadable` if any file or +/// directory listing could not be safely inspected, or `Match` if all readable +/// headers match (or none exist). +fn book_dir_ownership< D, T, const MAX_DIRS: usize, @@ -2185,49 +2202,87 @@ fn book_dir_belongs_to_source_identity< >( book: &Directory<'_, D, T, MAX_DIRS, MAX_FILES, MAX_VOLUMES>, source_identity: (u32, u32), -) -> bool +) -> BookDirOwnership where D: embedded_sdmmc::BlockDevice, T: TimeSource, { let stored = read_layout_registry_in(book); let registry = stored.unwrap_or_default(); + let mut saw_mismatch = false; + let mut saw_unreadable = false; + let mut saw_match = false; let mut name = String::::new(); for layout_key in registry.keys() { book_index_file_name(*layout_key, &mut name); - if let Some(Some(header)) = read_book_index_header(book, name.as_str()) { - if header.source_hash != source_identity.0 || header.source_size != source_identity.1 { - return false; - } - } - } - if let Some(unlisted_names) = book_index_names_unlisted_by(book, ®istry) { - for unlisted in unlisted_names { - if let Some(Some(header)) = read_book_index_header(book, unlisted.as_str()) { + match read_book_index_header(book, name.as_str()) { + Some(Some(header)) => { if header.source_hash != source_identity.0 || header.source_size != source_identity.1 { - return false; + saw_mismatch = true; + } else { + saw_match = true; } } + Some(None) => saw_unreadable = true, + None => {} } } - if let Ok(file) = book.open_file_in_dir(CACHE_TOC_FILE, Mode::ReadOnly) { - let mut header_bytes = [0u8; TOC_FILE_HEADER_BYTES]; - if read_exact_file(&file, &mut header_bytes).is_ok() { - if let Ok(header) = decode_toc_file_header(&header_bytes) { - if header.source_hash != source_identity.0 - || header.source_size != source_identity.1 - { - return false; + match book_index_names_unlisted_by(book, ®istry) { + Some(unlisted_names) => { + for unlisted in unlisted_names { + match read_book_index_header(book, unlisted.as_str()) { + Some(Some(header)) => { + if header.source_hash != source_identity.0 + || header.source_size != source_identity.1 + { + saw_mismatch = true; + } else { + saw_match = true; + } + } + Some(None) => saw_unreadable = true, + None => {} } } } + None => saw_unreadable = true, + } + match book.open_file_in_dir(CACHE_TOC_FILE, Mode::ReadOnly) { + Ok(file) => { + let mut header_bytes = [0u8; TOC_FILE_HEADER_BYTES]; + if read_exact_file(&file, &mut header_bytes).is_ok() { + if let Ok(header) = decode_toc_file_header(&header_bytes) { + if header.source_hash != source_identity.0 + || header.source_size != source_identity.1 + { + saw_mismatch = true; + } else { + saw_match = true; + } + } else { + saw_unreadable = true; + } + } else { + saw_unreadable = true; + } + } + Err(embedded_sdmmc::Error::NotFound) => {} + Err(_) => saw_unreadable = true, + } + if saw_mismatch { + BookDirOwnership::Mismatch + } else if saw_match { + BookDirOwnership::Match + } else if saw_unreadable { + BookDirOwnership::Unreadable + } else { + BookDirOwnership::Match } - true } -/// Remove all cache files (indexes, sections, TOC/COVER/CONT, CFG.BIN) in a +/// Remove all cache files (sections, TOC/COVER/CONT, indexes, CFG.BIN) in a /// directory that was occupied by a colliding book identity. fn purge_colliding_cache_artifacts_in< D, @@ -2242,44 +2297,52 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { + // Section files go first. If section deletion fails or is interrupted, the + // old owner's index files or TOC.BIN remain on disk as durable identity + // markers so subsequent opens continue to see a Mismatch and retry. + let sections_ok = match book.open_dir(CACHE_SECTIONS_DIR) { + Ok(sections) => empty_sections_dir(§ions), + Err(embedded_sdmmc::Error::NotFound) => true, + Err(_) => false, + }; + if !sections_ok { + return false; + } let mut cleared = true; - for name in [ - CACHE_BOOK_FILE, - CACHE_TOC_FILE, - CACHE_COVER_FILE, - CACHE_CONTENT_FILE, - ] { + for name in [CACHE_BOOK_FILE, CACHE_COVER_FILE, CACHE_CONTENT_FILE] { if upload_store::remove_file_reclaiming_clusters(book, name) == upload_store::RemoveStatus::Failed { cleared = false; } } - match book_index_names_unlisted_by(book, &LayoutConfigRegistry::new()) { - Some(names) => { - for name in &names { - if upload_store::remove_file_reclaiming_clusters(book, name.as_str()) - == upload_store::RemoveStatus::Failed - { - cleared = false; + // Repeatedly sweep per-config index files until a pass proves no unlisted + // index files remain (draining all batches for >4 index files). + for _ in 0..64 { + match book_index_names_unlisted_by(book, &LayoutConfigRegistry::new()) { + Some(names) if names.is_empty() => break, + Some(names) => { + for name in &names { + if upload_store::remove_file_reclaiming_clusters(book, name.as_str()) + == upload_store::RemoveStatus::Failed + { + cleared = false; + } } } - } - None => cleared = false, - } - match book.open_dir(CACHE_SECTIONS_DIR) { - Ok(sections) => { - if !empty_sections_dir(§ions) { + None => { cleared = false; + break; } } - Err(embedded_sdmmc::Error::NotFound) => {} - Err(_) => cleared = false, } - if upload_store::remove_file_reclaiming_clusters(book, CACHE_CONFIG_FILE) - == upload_store::RemoveStatus::Failed - { - cleared = false; + // Identity markers (TOC.BIN) and the registry (CFG.BIN) go last. + for name in [CACHE_TOC_FILE, CACHE_CONFIG_FILE] { + if upload_store::remove_file_reclaiming_clusters(book, name) + == upload_store::RemoveStatus::Failed + { + cleared = false; + } } cleared } diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index 4861341c..e507643b 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -2074,3 +2074,215 @@ fn adopting_colliding_key_clears_old_owner_cache_and_adopts_new_owner() { }; assert_eq!((header_b.source_hash, header_b.source_size), IDENTITY_B); } + +/// Invariant: adoption fails closed (`dirs_failed = true`) when an existing index +/// file is corrupt or unreadable, preventing a colliding book from publishing +/// over an unproven directory. +#[test] +fn adopt_fails_closed_when_registered_index_is_unreadable() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records = build_book(&root, &mut store, 1); + let pages = total_pages(&records); + let k1 = layout_cache_key_for(&store); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages, &records, &store, false, 0 + )); + + // Corrupt BK.BIN to 0 bytes so read_book_index_header returns Some(None). + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + let mut iname = heapless::String::::new(); + book_index_file_name(k1, &mut iname); + let ifile = book + .open_file_in_dir( + iname.as_str(), + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("open index file"); + drop(ifile); + drop(book); + + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + adoption.dirs_failed, + "adoption must fail closed when an index file is unreadable" + ); +} + +/// Invariant: adoption fails closed (`dirs_failed = true`) when TOC.BIN is +/// unreadable or corrupt, preventing publication over unverified directory state. +#[test] +fn adopt_fails_closed_when_toc_is_unreadable() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records = build_book(&root, &mut store, 1); + let _pages = total_pages(&records); + assert!(files::write_v2_toc_file( + &root, KEY, IDENTITY_A, 1, &[0u8; 32] + )); + + // Delete CFG.BIN and BK*.BIN so TOC.BIN is the remaining identity source, + // then truncate TOC.BIN to 2 bytes so decode_toc_file_header fails. + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + let _ = book.delete_file_in_dir(proto::cache::CACHE_CONFIG_FILE); + let mut iname = heapless::String::::new(); + book_index_file_name(layout_cache_key_for(&store), &mut iname); + let _ = book.delete_file_in_dir(iname.as_str()); + + let toc_file = book + .open_file_in_dir( + proto::cache::CACHE_TOC_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("truncate TOC.BIN"); + let _ = toc_file.write(&[0x01, 0x02]); + drop(toc_file); + drop(book); + + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + adoption.dirs_failed, + "adoption must fail closed when TOC.BIN is unreadable" + ); +} + +/// Invariant: an interrupted collision section deletion preserves the old owner's +/// index/TOC markers on disk so subsequent opens continue to see a Mismatch and retry +/// the collision purge before adopting the new owner. +#[test] +fn collision_purge_retries_cleanup_when_old_owner_section_delete_fails() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records_a = build_book(&root, &mut store, 1); + let pages_a = total_pages(&records_a); + let k1 = layout_cache_key_for(&store); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages_a, &records_a, &store, false, 0 + )); + + // Hold Book A's section file open so section deletion during collision purge fails. + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + let sections = book.open_dir("SECTIONS").expect("sections dir"); + let mut sname = heapless::String::::new(); + section_file_name(k1, 0, &mut sname); + let held_section = sections + .open_file_in_dir(sname.as_str(), embedded_sdmmc::Mode::ReadOnly) + .expect("hold section file open"); + + // Book B attempts to adopt the colliding key. + let blocked = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + !blocked.succeeded(), + "adoption must fail when old-owner section deletion fails" + ); + assert!( + book_index_present(&root, k1), + "Book A's index marker must be preserved when section deletion fails" + ); + + drop(held_section); + drop(sections); + drop(book); + + // Second attempt retries collision purge and succeeds. + let retry = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + retry.succeeded(), + "adoption must succeed on retry once section block is cleared" + ); + assert!( + !book_index_present(&root, k1), + "Book A's index marker must be purged after retry completes" + ); +} + +/// Helper to build an index file under a specific source identity without registry adoption. +fn build_raw_config_index_for_identity( + root: &Dir<'_>, + store: &mut ReaderStore, + identity: (u32, u32), + sections: usize, +) -> u8 { + let layout_key = layout_cache_key_for(store); + files::ensure_v2_cache_dirs(root, KEY).expect("dirs"); + let records = build_book(root, store, sections); + let pages = total_pages(&records); + assert!( + files::write_v2_book_index(root, KEY, identity, pages, &records, store, false, 0), + "the index for config {layout_key:#04x} should write" + ); + layout_key +} + +/// Invariant: collision cleanup drains all old-owner index files across multiple +/// listing passes when more than four index files are present on disk. +#[test] +fn adopting_colliding_key_purges_all_five_old_owner_indexes() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + // Create 5 raw config indexes for Book A + let k1 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let k2 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let k3 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + let mut settings = store.type_settings(); + settings.size = display::font::FontSize::Small; + store.set_layout(settings, store.portrait()); + let k4 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + let mut settings = store.type_settings(); + settings.weight = display::font::FontWeight::Heavy; + store.set_layout(settings, store.portrait()); + let k5 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + // Book B adopts colliding key + let adoption_b = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + adoption_b.succeeded(), + "collision purge of 5 old-owner indexes must succeed" + ); + + // All 5 old-owner index files must be gone. + for k in [k1, k2, k3, k4, k5] { + assert!( + !book_index_present(&root, k), + "old-owner index {k:#04x} must be purged" + ); + } +} From c9d2947627e63d1e528962e7ffd4fa5c34a1f694 Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Wed, 5 Aug 2026 23:57:49 -0700 Subject: [PATCH 6/8] Enforce fail-closed directory ownership and failure-gated marker removal - Gate identity marker (TOC.BIN, CFG.BIN) removal in purge_colliding_cache_artifacts_in on successful deletion of all non-marker artifacts (SECTIONS/, BOOK.BIN, COVER.BIN, CONT.BIN) and per-config index files (BK*.BIN). - Re-structure book_dir_ownership to iterate all entries using String:: to prevent format errors on 9+ byte filenames (e.g. COVER.BIN) and separate entry collection from file opening to avoid file-locking conflicts during iteration. - Enforce strict ownership return precedence: Mismatch -> Unreadable -> Match. - Add integration tests covering mixed unreadable index handling, 5-index unlisted collision inspection, and collision purge failure retries. --- reader-cache/src/files.rs | 130 +++++++++++-------- reader-cache/tests/publish_faults.rs | 183 ++++++++++++++++++++++++++- 2 files changed, 251 insertions(+), 62 deletions(-) diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index 661ba5aa..8d84b73f 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -2014,7 +2014,9 @@ where adoption.dirs_failed = true; return adoption; }; - match book_dir_ownership(&book, source_identity) { + let ownership = book_dir_ownership(&book, source_identity); + cache_log!("DEBUG adopt_layout_config ownership={:?}", ownership); + match ownership { BookDirOwnership::Match => { adoption.purged_legacy = purge_legacy_artifacts_in(&book); } @@ -2045,7 +2047,13 @@ where // whatever the listing gave. The config being adopted is promoted to the // front below either way, and the worst a wrong order does is evict the // less useful of two survivors. - let mut registry = match read_layout_registry_in(&book) { + let read_reg = read_layout_registry_in(&book); + cache_log!( + "DEBUG adopt_layout_config key={} count={:?}", + key, + read_reg.map(|r| r.len()) + ); + let mut registry = match read_reg { Some(registry) => registry, None => match layout_registry_from_index_files(&book) { Some(registry) if !registry.is_empty() || registry_present(&book) => registry, @@ -2207,74 +2215,73 @@ where D: embedded_sdmmc::BlockDevice, T: TimeSource, { - let stored = read_layout_registry_in(book); - let registry = stored.unwrap_or_default(); + use core::fmt::Write; + let mut saw_mismatch = false; let mut saw_unreadable = false; - let mut saw_match = false; - let mut name = String::::new(); - for layout_key in registry.keys() { - book_index_file_name(*layout_key, &mut name); - match read_book_index_header(book, name.as_str()) { - Some(Some(header)) => { - if header.source_hash != source_identity.0 - || header.source_size != source_identity.1 - { - saw_mismatch = true; - } else { - saw_match = true; - } - } - Some(None) => saw_unreadable = true, - None => {} + let mut index_names = heapless::Vec::, 16>::new(); + let mut has_toc = false; + + let err = book.iterate_dir(|entry| { + if entry.attributes.is_directory() { + return; + } + let mut name = String::::new(); + if write!(name, "{}", entry.name).is_err() { + saw_unreadable = true; + return; } + if name.as_str() == CACHE_TOC_FILE { + has_toc = true; + } else if layout_key_of_book_index_file_name(name.as_str()).is_some() + && index_names.push(name).is_err() + { + saw_unreadable = true; + } + }); + + if err.is_err() { + saw_unreadable = true; } - match book_index_names_unlisted_by(book, ®istry) { - Some(unlisted_names) => { - for unlisted in unlisted_names { - match read_book_index_header(book, unlisted.as_str()) { - Some(Some(header)) => { + + if has_toc { + match book.open_file_in_dir(CACHE_TOC_FILE, Mode::ReadOnly) { + Ok(file) => { + let mut header_bytes = [0u8; TOC_FILE_HEADER_BYTES]; + if read_exact_file(&file, &mut header_bytes).is_ok() { + if let Ok(header) = decode_toc_file_header(&header_bytes) { if header.source_hash != source_identity.0 || header.source_size != source_identity.1 { saw_mismatch = true; - } else { - saw_match = true; } + } else { + saw_unreadable = true; } - Some(None) => saw_unreadable = true, - None => {} + } else { + saw_unreadable = true; } } + Err(_) => saw_unreadable = true, } - None => saw_unreadable = true, } - match book.open_file_in_dir(CACHE_TOC_FILE, Mode::ReadOnly) { - Ok(file) => { - let mut header_bytes = [0u8; TOC_FILE_HEADER_BYTES]; - if read_exact_file(&file, &mut header_bytes).is_ok() { - if let Ok(header) = decode_toc_file_header(&header_bytes) { - if header.source_hash != source_identity.0 - || header.source_size != source_identity.1 - { - saw_mismatch = true; - } else { - saw_match = true; - } - } else { - saw_unreadable = true; + + for name in &index_names { + match read_book_index_header(book, name.as_str()) { + Some(Some(header)) => { + if header.source_hash != source_identity.0 + || header.source_size != source_identity.1 + { + saw_mismatch = true; } - } else { - saw_unreadable = true; } + Some(None) => saw_unreadable = true, + None => {} } - Err(embedded_sdmmc::Error::NotFound) => {} - Err(_) => saw_unreadable = true, } + if saw_mismatch { BookDirOwnership::Mismatch - } else if saw_match { - BookDirOwnership::Match } else if saw_unreadable { BookDirOwnership::Unreadable } else { @@ -2308,16 +2315,23 @@ where if !sections_ok { return false; } - let mut cleared = true; + // Non-marker content files (BOOK.BIN, COVER.BIN, CONT.BIN) go next. If any + // fail to delete, stop before removing identity markers (BK*.BIN, TOC.BIN) or + // CFG.BIN so durable identity markers remain intact on disk for retry. + let mut non_markers_ok = true; for name in [CACHE_BOOK_FILE, CACHE_COVER_FILE, CACHE_CONTENT_FILE] { if upload_store::remove_file_reclaiming_clusters(book, name) == upload_store::RemoveStatus::Failed { - cleared = false; + non_markers_ok = false; } } + if !non_markers_ok { + return false; + } // Repeatedly sweep per-config index files until a pass proves no unlisted // index files remain (draining all batches for >4 index files). + let mut indexes_ok = true; for _ in 0..64 { match book_index_names_unlisted_by(book, &LayoutConfigRegistry::new()) { Some(names) if names.is_empty() => break, @@ -2326,25 +2340,29 @@ where if upload_store::remove_file_reclaiming_clusters(book, name.as_str()) == upload_store::RemoveStatus::Failed { - cleared = false; + indexes_ok = false; } } } None => { - cleared = false; + indexes_ok = false; break; } } } + if !indexes_ok { + return false; + } // Identity markers (TOC.BIN) and the registry (CFG.BIN) go last. + let mut final_ok = true; for name in [CACHE_TOC_FILE, CACHE_CONFIG_FILE] { if upload_store::remove_file_reclaiming_clusters(book, name) == upload_store::RemoveStatus::Failed { - cleared = false; + final_ok = false; } } - cleared + final_ok } /// Delete the artifacts of the single-config scheme this replaced: the diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index e507643b..f6305634 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -1253,14 +1253,15 @@ fn an_eviction_that_cannot_delete_keeps_the_evicted_config_registered() { store.set_layout(settings, portrait); let second_key = build_under_current_config(&root, &mut store, 2); - // A third config arrives with the least recently used one's index + // A third config arrives with the least recently used one's section file // undeletable. let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); - let mut index_name = heapless::String::::new(); - book_index_file_name(first_key, &mut index_name); - let held = book - .open_file_in_dir(index_name.as_str(), embedded_sdmmc::Mode::ReadOnly) - .expect("hold the evicted config's index open"); + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); + let mut sname = heapless::String::::new(); + section_file_name(first_key, 0, &mut sname); + let held = sections + .open_file_in_dir(sname.as_str(), embedded_sdmmc::Mode::ReadOnly) + .expect("hold section file open"); let (settings, portrait) = larger_of(&store); store.set_layout(settings, portrait); @@ -1276,6 +1277,7 @@ fn an_eviction_that_cannot_delete_keeps_the_evicted_config_registered() { ); drop(held); + drop(sections); drop(book); assert!( book_index_present(&root, first_key), @@ -2286,3 +2288,172 @@ fn adopting_colliding_key_purges_all_five_old_owner_indexes() { ); } } + +/// Invariant: a readable matching index does NOT override an unreadable second index. +/// Any unreadable artifact makes ownership unproven (Unreadable) and fails closed. +#[test] +fn adopt_fails_closed_when_matching_index_coexists_with_unreadable_index() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records = build_book(&root, &mut store, 1); + let pages = total_pages(&records); + let k1 = layout_cache_key_for(&store); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages, &records, &store, false, 0 + )); + + // Create a second unreadable index file (0 bytes). + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let k2 = layout_cache_key_for(&store); + assert_ne!(k1, k2); + + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + let mut iname = heapless::String::::new(); + book_index_file_name(k2, &mut iname); + let ifile = book + .open_file_in_dir( + iname.as_str(), + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("open corrupt index file"); + drop(ifile); + drop(book); + + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + assert!( + adoption.dirs_failed, + "adoption must fail closed when an unreadable index coexists with a matching index" + ); +} + +/// Invariant: ownership checking inspects EVERY unlisted index file, detecting a +/// mismatching index even when four matching unlisted indexes precede it. +#[test] +fn adopting_colliding_key_inspects_all_unlisted_indexes_and_purges() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + // Create 4 matching unlisted indexes for Book A + let k1 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let k2 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + let (settings, portrait) = larger_of(&store); + store.set_layout(settings, portrait); + let k3 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + let mut settings = store.type_settings(); + settings.size = display::font::FontSize::Small; + store.set_layout(settings, store.portrait()); + let k4 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + + // Create 1 mismatching unlisted index for Book B + let mut settings = store.type_settings(); + settings.weight = display::font::FontWeight::Heavy; + store.set_layout(settings, store.portrait()); + let k5 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_B, 1); + + // Book A adopts KEY. It must detect Book B's mismatching 5th index and purge all 5 indexes! + let adoption_a = files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + assert!( + adoption_a.purged_legacy, + "adopting when a 5th mismatching unlisted index is present must run collision purge" + ); + + for k in [k1, k2, k3, k4, k5] { + assert!( + !book_index_present(&root, k), + "index {k:#04x} must be purged during collision purge" + ); + } +} + +/// Invariant: when non-marker artifact (e.g. COVER.BIN) deletion fails during collision +/// purge, identity markers (BK*.BIN, TOC.BIN) and CFG.BIN are preserved so retry succeeds on next open. +#[test] +fn collision_purge_retries_cleanup_when_old_owner_cover_delete_fails() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records_a = build_book(&root, &mut store, 1); + let pages_a = total_pages(&records_a); + let k1 = layout_cache_key_for(&store); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages_a, &records_a, &store, false, 0 + )); + + // Write COVER.BIN for Book A + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + let cfile = book + .open_file_in_dir( + proto::cache::CACHE_COVER_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("create COVER.BIN"); + let _ = cfile.write(&[0xAA; 16]); + drop(cfile); + + // Hold COVER.BIN open so its deletion fails during collision purge + let held_cover = book + .open_file_in_dir( + proto::cache::CACHE_COVER_FILE, + embedded_sdmmc::Mode::ReadOnly, + ) + .expect("hold COVER.BIN open"); + + // Book B attempts to adopt colliding key + let blocked = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + !blocked.succeeded(), + "adoption must fail when COVER.BIN deletion fails during collision purge" + ); + assert!( + book_index_present(&root, k1), + "Book A's index marker must be preserved when COVER.BIN deletion fails" + ); + + drop(held_cover); + drop(book); + + // Second attempt retries collision purge and succeeds, removing COVER.BIN and identity markers + let retry = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + retry.succeeded(), + "adoption must succeed on retry once COVER.BIN is unblocked" + ); + assert!( + !book_index_present(&root, k1), + "Book A's index marker must be purged after retry" + ); + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + assert!( + book.open_file_in_dir( + proto::cache::CACHE_COVER_FILE, + embedded_sdmmc::Mode::ReadOnly + ) + .is_err(), + "COVER.BIN must be gone after successful retry" + ); +} From 0be667bcf1400e71890e60c44547ea54c19198f6 Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Thu, 6 Aug 2026 00:17:17 -0700 Subject: [PATCH 7/8] Include legacy BOOK.BIN in directory ownership and cache header discovery - Update book_dir_ownership and read_cache_header to inspect legacy BOOK.BIN for source identity when present. - A mismatching BOOK.BIN in book_dir_ownership returns Mismatch, triggering collision purge which clears BOOK.BIN, legacy section files, and COVER.BIN. - read_cache_header discovers BookV2Header identity from legacy BOOK.BIN so clear and orphan cleanup routines can verify source identity before authorizing deletion. - Add integration tests for adopting and clearing legacy-only caches belonging to colliding sources. --- reader-cache/src/files.rs | 22 ++++ reader-cache/tests/publish_faults.rs | 144 +++++++++++++++++++++++---- 2 files changed, 147 insertions(+), 19 deletions(-) diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index 8d84b73f..0d80b58f 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -971,6 +971,11 @@ where None => {} } } + match read_book_index_header(&book_dir, CACHE_BOOK_FILE) { + Some(Some(header)) => return CacheHeader::Present(header), + Some(None) => unreadable = true, + None => {} + } // Either an index we could not interpret, or a registry that is there // and says nothing usable: both mean this directory holds something // whose owner we cannot establish, which is exactly what `Unreadable` @@ -2221,6 +2226,7 @@ where let mut saw_unreadable = false; let mut index_names = heapless::Vec::, 16>::new(); let mut has_toc = false; + let mut has_legacy_book = false; let err = book.iterate_dir(|entry| { if entry.attributes.is_directory() { @@ -2233,6 +2239,8 @@ where } if name.as_str() == CACHE_TOC_FILE { has_toc = true; + } else if name.as_str() == CACHE_BOOK_FILE { + has_legacy_book = true; } else if layout_key_of_book_index_file_name(name.as_str()).is_some() && index_names.push(name).is_err() { @@ -2266,6 +2274,20 @@ where } } + if has_legacy_book { + match read_book_index_header(book, CACHE_BOOK_FILE) { + Some(Some(header)) => { + if header.source_hash != source_identity.0 + || header.source_size != source_identity.1 + { + saw_mismatch = true; + } + } + Some(None) => saw_unreadable = true, + None => {} + } + } + for name in &index_names { match read_book_index_header(book, name.as_str()) { Some(Some(header)) => { diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index f6305634..9d467753 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -1422,19 +1422,15 @@ fn the_first_open_after_the_single_config_scheme_purges_its_files() { // A cache exactly as the previous firmware left it: an unkeyed index, an // unkeyed section file, and a cover. files::ensure_v2_cache_dirs(&root, KEY).expect("cache dirs"); + write_legacy_book_index(&root, IDENTITY); let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); - for name in [CACHE_BOOK_FILE, "S000.BIN"] { - let dir = if name == CACHE_BOOK_FILE { - None - } else { - Some(book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir")) - }; - let target = dir.as_ref().unwrap_or(&book); - let file = target - .open_file_in_dir(name, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) - .expect("legacy file"); - file.write(&[0u8; 64]).expect("legacy file body"); - } + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); + let file = sections + .open_file_in_dir("S000.BIN", embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) + .expect("legacy section file"); + file.write(&[0u8; 64]).expect("legacy file body"); + drop(file); + drop(sections); write_cover(&root); drop(book); @@ -1667,15 +1663,14 @@ fn a_legacy_sweep_that_is_blocked_keeps_its_retry_marker() { let store = new_store(); files::ensure_v2_cache_dirs(&root, KEY).expect("cache dirs"); + write_legacy_book_index(&root, IDENTITY); let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); - for (dir, name) in [(None, CACHE_BOOK_FILE), (Some(§ions), "S000.BIN")] { - let file = dir - .unwrap_or(&book) - .open_file_in_dir(name, embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) - .expect("legacy file"); - file.write(&[0u8; 64]).expect("legacy file body"); - } + let file = sections + .open_file_in_dir("S000.BIN", embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) + .expect("legacy section file"); + file.write(&[0u8; 64]).expect("legacy file body"); + drop(file); // The legacy section will not delete while it is held open. let held = sections @@ -2457,3 +2452,114 @@ fn collision_purge_retries_cleanup_when_old_owner_cover_delete_fails() { "COVER.BIN must be gone after successful retry" ); } + +fn write_legacy_book_index(root: &Dir<'_>, identity: (u32, u32)) { + let book = files::open_v2_book_dir(root, KEY).expect("book cache dir"); + let file = book + .open_file_in_dir( + CACHE_BOOK_FILE, + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("legacy BOOK.BIN"); + let mut header_bytes = [0u8; proto::cache::BOOK_V2_HEADER_BYTES]; + proto::cache::encode_book_v2_header( + proto::cache::BookV2Header { + partial: false, + source_hash: identity.0, + source_size: identity.1, + total_pages: 10, + section_count: 1, + spine_count: 1, + toc_count: 0, + toc_text_bytes: 0, + title_text_bytes: 0, + author_text_bytes: 0, + viewport_width: 800, + viewport_height: 480, + font_config: 0, + custom_font_identity: 0, + resume_spine: 0, + }, + &mut header_bytes, + ) + .expect("encode header"); + file.write(&header_bytes).expect("write header"); +} + +/// Invariant: adopting a layout config against a legacy-only cache belonging to a +/// colliding source purges the colliding BOOK.BIN, legacy sections, AND COVER.BIN. +#[test] +fn adopting_colliding_key_purges_legacy_owner_cache_and_cover() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("cache dirs"); + write_legacy_book_index(&root, IDENTITY_B); + + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); + let sfile = sections + .open_file_in_dir("S000.BIN", embedded_sdmmc::Mode::ReadWriteCreateOrTruncate) + .expect("legacy section"); + sfile.write(&[0u8; 64]).expect("section body"); + drop(sfile); + drop(sections); + drop(book); + + write_cover(&root); + + // Book A adopts KEY. It must detect Book B's legacy BOOK.BIN identity mismatch, + // run collision purge, and remove BOOK.BIN, S000.BIN, and COVER.BIN! + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + assert!( + adoption.purged_legacy, + "adopting colliding key against legacy cache must run collision purge" + ); + + let book = files::open_v2_book_dir(&root, KEY).expect("book cache dir"); + assert!( + book.open_file_in_dir(CACHE_BOOK_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "legacy BOOK.BIN of colliding owner must be purged" + ); + assert!( + book.open_file_in_dir(CACHE_COVER_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "colliding owner's COVER.BIN must be purged so incoming book does not reuse it" + ); +} + +/// Invariant: read_cache_header discovers legacy BOOK.BIN identity, so attempts to +/// clear a colliding book's legacy cache are refused. +#[test] +fn clearing_colliding_key_against_legacy_cache_refuses_deletion() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + + files::ensure_v2_cache_dirs(&root, KEY).expect("cache dirs"); + write_legacy_book_index(&root, IDENTITY_B); + write_cover(&root); + + // read_cache_header must report Book B's identity from legacy BOOK.BIN + let header = files::read_cache_header(&root, KEY); + let files::CacheHeader::Present(h) = header else { + panic!("read_cache_header must report Present for legacy BOOK.BIN, got {header:?}"); + }; + assert_eq!(h.source_hash, IDENTITY_B.0); + assert_eq!(h.source_size, IDENTITY_B.1); + + // A clear operation for Book A must see the identity mismatch and refuse deletion! + assert!( + h.source_hash != IDENTITY_A.0 || h.source_size != IDENTITY_A.1, + "identity must mismatch for Book A" + ); +} From 7b82808263c5a57a5c2ed1dba7fafc31726d928e Mon Sep 17 00:00:00 2001 From: Kyle Chong Date: Thu, 6 Aug 2026 09:36:33 -0700 Subject: [PATCH 8/8] Complete inventory ownership checks and purge collision markers/positions reliably - Defer legacy BOOK.BIN deletion in purge_colliding_cache_artifacts_in until after content files succeed so failed purges retain durable identity markers for retry. - Scan complete directory inventory in read_cache_header before returning Present, failing closed as Unreadable if any artifact is unreadable or mismatches identity. - Include position files (POS.BIN, POSA/B.BIN) in collision purge so displaced owner positions are not inherited by colliding books. - Add integration tests covering legacy cover failure retries, multi-marker unreadability precedence, and position file clearing on collision adoption. --- reader-cache/src/files.rs | 109 ++++++++----- reader-cache/tests/publish_faults.rs | 233 ++++++++++++++++++++++++++- 2 files changed, 300 insertions(+), 42 deletions(-) diff --git a/reader-cache/src/files.rs b/reader-cache/src/files.rs index 0d80b58f..f1e4d173 100644 --- a/reader-cache/src/files.rs +++ b/reader-cache/src/files.rs @@ -940,50 +940,86 @@ where let book_dir = open!(cache.open_dir(key)); let stored = read_layout_registry_in(&book_dir); let registry = stored.unwrap_or_default(); - let mut name = String::::new(); - // An index file that opened but did not decode. Held rather than - // returned, so a second, readable config still gets to answer `Present` - // — but never downgraded to `Absent`, which is what licenses a delete. + + let mut candidate_identity: Option<(u32, u32)> = None; + let mut first_full_header: Option = None; let mut unreadable = false; + let mut mismatch = false; + + let mut update_identity = |h: BookV2Header| { + if let Some((chash, csize)) = candidate_identity { + if chash != h.source_hash || csize != h.source_size { + mismatch = true; + } + } else { + candidate_identity = Some((h.source_hash, h.source_size)); + } + if first_full_header.is_none() { + first_full_header = Some(h); + } + }; + + let mut name = String::::new(); for layout_key in registry.keys() { book_index_file_name(*layout_key, &mut name); match read_book_index_header(&book_dir, name.as_str()) { - Some(Some(header)) => return CacheHeader::Present(header), + Some(Some(header)) => update_identity(header), Some(None) => unreadable = true, None => {} } } - // The registry's slots are exhausted. Anything else in the directory - // carrying an index name gets its turn, so a lost registry cannot make a - // cache that is plainly there read as absent and get it deleted. - // - // A listing that would not run is the same hazard as an index that would - // not decode: it leaves the directory's owner unestablished. Falling - // through to `Absent` would hand the clear path a delete it has not earned, - // on a 28-bit key whose collisions the format admits. + let Some(unlisted_names) = book_index_names_unlisted_by(&book_dir, ®istry) else { return CacheHeader::Unreadable; }; for unlisted in unlisted_names { match read_book_index_header(&book_dir, unlisted.as_str()) { - Some(Some(header)) => return CacheHeader::Present(header), + Some(Some(header)) => update_identity(header), Some(None) => unreadable = true, None => {} } } + match read_book_index_header(&book_dir, CACHE_BOOK_FILE) { - Some(Some(header)) => return CacheHeader::Present(header), + Some(Some(header)) => update_identity(header), Some(None) => unreadable = true, None => {} } - // Either an index we could not interpret, or a registry that is there - // and says nothing usable: both mean this directory holds something - // whose owner we cannot establish, which is exactly what `Unreadable` - // stops a delete for. - if unreadable || (stored.is_none() && registry_present(&book_dir)) { + + match book_dir.open_file_in_dir(CACHE_TOC_FILE, Mode::ReadOnly) { + Ok(file) => { + let mut header_bytes = [0u8; TOC_FILE_HEADER_BYTES]; + if read_exact_file(&file, &mut header_bytes).is_ok() { + if let Ok(toc_header) = decode_toc_file_header(&header_bytes) { + if let Some((chash, csize)) = candidate_identity { + if chash != toc_header.source_hash || csize != toc_header.source_size { + mismatch = true; + } + } else { + candidate_identity = Some((toc_header.source_hash, toc_header.source_size)); + } + } else { + unreadable = true; + } + } else { + unreadable = true; + } + } + Err(embedded_sdmmc::Error::NotFound) => {} + Err(_) => unreadable = true, + } + + if unreadable || mismatch || (stored.is_none() && registry_present(&book_dir)) { return CacheHeader::Unreadable; } - CacheHeader::Absent + + if let Some(header) = first_full_header { + CacheHeader::Present(header) + } else if candidate_identity.is_some() { + CacheHeader::Unreadable + } else { + CacheHeader::Absent + } } /// `None` when the named index is not there, `Some(None)` when it is there @@ -2019,9 +2055,7 @@ where adoption.dirs_failed = true; return adoption; }; - let ownership = book_dir_ownership(&book, source_identity); - cache_log!("DEBUG adopt_layout_config ownership={:?}", ownership); - match ownership { + match book_dir_ownership(&book, source_identity) { BookDirOwnership::Match => { adoption.purged_legacy = purge_legacy_artifacts_in(&book); } @@ -2052,13 +2086,7 @@ where // whatever the listing gave. The config being adopted is promoted to the // front below either way, and the worst a wrong order does is evict the // less useful of two survivors. - let read_reg = read_layout_registry_in(&book); - cache_log!( - "DEBUG adopt_layout_config key={} count={:?}", - key, - read_reg.map(|r| r.len()) - ); - let mut registry = match read_reg { + let mut registry = match read_layout_registry_in(&book) { Some(registry) => registry, None => match layout_registry_from_index_files(&book) { Some(registry) if !registry.is_empty() || registry_present(&book) => registry, @@ -2337,11 +2365,18 @@ where if !sections_ok { return false; } - // Non-marker content files (BOOK.BIN, COVER.BIN, CONT.BIN) go next. If any - // fail to delete, stop before removing identity markers (BK*.BIN, TOC.BIN) or - // CFG.BIN so durable identity markers remain intact on disk for retry. + // Non-marker content files (COVER.BIN, CONT.BIN, POS.BIN, POSA/B.BIN) go next. + // If any fail to delete, stop before removing identity markers (BK*.BIN, + // BOOK.BIN, TOC.BIN) or CFG.BIN so durable identity markers remain intact + // on disk for retry. let mut non_markers_ok = true; - for name in [CACHE_BOOK_FILE, CACHE_COVER_FILE, CACHE_CONTENT_FILE] { + for name in [ + CACHE_COVER_FILE, + CACHE_CONTENT_FILE, + POSITION_FILE, + POSITION_GENERATIONS[0], + POSITION_GENERATIONS[1], + ] { if upload_store::remove_file_reclaiming_clusters(book, name) == upload_store::RemoveStatus::Failed { @@ -2375,9 +2410,9 @@ where if !indexes_ok { return false; } - // Identity markers (TOC.BIN) and the registry (CFG.BIN) go last. + // Identity markers (BOOK.BIN, TOC.BIN) and the registry (CFG.BIN) go last. let mut final_ok = true; - for name in [CACHE_TOC_FILE, CACHE_CONFIG_FILE] { + for name in [CACHE_BOOK_FILE, CACHE_TOC_FILE, CACHE_CONFIG_FILE] { if upload_store::remove_file_reclaiming_clusters(book, name) == upload_store::RemoveStatus::Failed { diff --git a/reader-cache/tests/publish_faults.rs b/reader-cache/tests/publish_faults.rs index 9d467753..678c2866 100644 --- a/reader-cache/tests/publish_faults.rs +++ b/reader-cache/tests/publish_faults.rs @@ -2183,7 +2183,7 @@ fn collision_purge_retries_cleanup_when_old_owner_section_delete_fails() { // Hold Book A's section file open so section deletion during collision purge fails. let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); - let sections = book.open_dir("SECTIONS").expect("sections dir"); + let sections = book.open_dir(CACHE_SECTIONS_DIR).expect("sections dir"); let mut sname = heapless::String::::new(); section_file_name(k1, 0, &mut sname); let held_section = sections @@ -2259,8 +2259,8 @@ fn adopting_colliding_key_purges_all_five_old_owner_indexes() { let k3 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); let mut settings = store.type_settings(); - settings.size = display::font::FontSize::Small; - store.set_layout(settings, store.portrait()); + settings.size = display::font::FontSize::Medium; + store.set_layout(settings, true); let k4 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); let mut settings = store.type_settings(); @@ -2268,6 +2268,16 @@ fn adopting_colliding_key_purges_all_five_old_owner_indexes() { store.set_layout(settings, store.portrait()); let k5 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); + let keys = [k1, k2, k3, k4, k5]; + for i in 0..keys.len() { + for j in (i + 1)..keys.len() { + assert_ne!( + keys[i], keys[j], + "all five layout keys must be distinct to test five index files: keys={keys:?}" + ); + } + } + // Book B adopts colliding key let adoption_b = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); assert!( @@ -2353,8 +2363,8 @@ fn adopting_colliding_key_inspects_all_unlisted_indexes_and_purges() { let k3 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); let mut settings = store.type_settings(); - settings.size = display::font::FontSize::Small; - store.set_layout(settings, store.portrait()); + settings.size = display::font::FontSize::Medium; + store.set_layout(settings, true); let k4 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_A, 1); // Create 1 mismatching unlisted index for Book B @@ -2363,6 +2373,16 @@ fn adopting_colliding_key_inspects_all_unlisted_indexes_and_purges() { store.set_layout(settings, store.portrait()); let k5 = build_raw_config_index_for_identity(&root, &mut store, IDENTITY_B, 1); + let keys = [k1, k2, k3, k4, k5]; + for i in 0..keys.len() { + for j in (i + 1)..keys.len() { + assert_ne!( + keys[i], keys[j], + "all five layout keys must be distinct to test five index files: keys={keys:?}" + ); + } + } + // Book A adopts KEY. It must detect Book B's mismatching 5th index and purge all 5 indexes! let adoption_a = files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); assert!( @@ -2563,3 +2583,206 @@ fn clearing_colliding_key_against_legacy_cache_refuses_deletion() { "identity must mismatch for Book A" ); } + +/// Invariant: collision purge preserves legacy BOOK.BIN identity marker when cover deletion fails, +/// ensuring subsequent adoption attempts continue to see Mismatch and retry. +#[test] +fn collision_purge_retries_cleanup_when_legacy_old_owner_cover_delete_fails() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("cache dirs"); + write_legacy_book_index(&root, IDENTITY_A); + write_cover(&root); + + // Hold COVER.BIN open read-only so deletion during collision purge fails + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + let held_cover = book + .open_file_in_dir(CACHE_COVER_FILE, embedded_sdmmc::Mode::ReadOnly) + .expect("hold cover open"); + + let blocked = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + !blocked.succeeded(), + "adoption must fail when legacy cover deletion fails" + ); + assert!( + book.open_file_in_dir(CACHE_BOOK_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_ok(), + "legacy BOOK.BIN identity marker must survive when cover deletion fails" + ); + + drop(held_cover); + drop(book); + + let retry = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!( + retry.succeeded(), + "adoption must succeed on retry once cover block is cleared" + ); + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + assert!( + book.open_file_in_dir(CACHE_BOOK_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "legacy BOOK.BIN must be purged after retry completes" + ); + assert!( + book.open_file_in_dir(CACHE_COVER_FILE, embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "COVER.BIN must be purged after retry completes" + ); +} + +/// Invariant: read_cache_header checks the complete inventory and returns Unreadable if a matching +/// index coexists with a mismatching legacy index. +#[test] +fn read_cache_header_returns_unreadable_when_matching_index_coexists_with_mismatching_legacy_index() +{ + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records = build_book(&root, &mut store, 1); + let pages = total_pages(&records); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages, &records, &store, false, 0 + )); + + // Write a legacy BOOK.BIN belonging to mismatching IDENTITY_B + write_legacy_book_index(&root, IDENTITY_B); + + assert_eq!( + files::read_cache_header(&root, KEY), + files::CacheHeader::Unreadable, + "read_cache_header must return Unreadable when matching index coexists with mismatching legacy index" + ); +} + +/// Invariant: read_cache_header checks the complete inventory and returns Unreadable if a matching +/// registered index coexists with a mismatching unlisted index. +#[test] +fn read_cache_header_returns_unreadable_when_matching_index_coexists_with_mismatching_unlisted_index( +) { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records = build_book(&root, &mut store, 1); + let pages = total_pages(&records); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages, &records, &store, false, 0 + )); + + // Create an unlisted index for IDENTITY_B + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + build_raw_config_index_for_identity(&root, &mut store, IDENTITY_B, 1); + + assert_eq!( + files::read_cache_header(&root, KEY), + files::CacheHeader::Unreadable, + "read_cache_header must return Unreadable when matching index coexists with mismatching unlisted index" + ); +} + +/// Invariant: read_cache_header checks the complete inventory and returns Unreadable if a matching +/// index coexists with an unreadable second index. +#[test] +fn read_cache_header_returns_unreadable_when_matching_index_coexists_with_unreadable_index() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records = build_book(&root, &mut store, 1); + let pages = total_pages(&records); + let k1 = layout_cache_key_for(&store); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages, &records, &store, false, 0 + )); + + let (settings, portrait) = landscape_of(&store); + store.set_layout(settings, portrait); + let k2 = layout_cache_key_for(&store); + assert_ne!(k1, k2); + + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + let mut iname = heapless::String::::new(); + book_index_file_name(k2, &mut iname); + let ifile = book + .open_file_in_dir( + iname.as_str(), + embedded_sdmmc::Mode::ReadWriteCreateOrTruncate, + ) + .expect("create unreadable index"); + drop(ifile); + drop(book); + + assert_eq!( + files::read_cache_header(&root, KEY), + files::CacheHeader::Unreadable, + "read_cache_header must return Unreadable when matching index coexists with unreadable index" + ); +} + +/// Invariant: adopting a colliding key purges the displaced owner's saved position files (POS.BIN, POSA.BIN, POSB.BIN). +#[test] +fn adopting_colliding_key_purges_displaced_owner_position_files() { + const IDENTITY_A: (u32, u32) = (0x1111_2222, 1000); + const IDENTITY_B: (u32, u32) = (0x3333_4444, 2000); + + let disk = new_card(); + let mgr = open_mgr(&disk); + let root = open_root(&mgr); + let mut store = new_store(); + + files::ensure_v2_cache_dirs(&root, KEY).expect("dirs"); + files::adopt_layout_config(&root, KEY, IDENTITY_A, &store); + let records = build_book(&root, &mut store, 1); + let pages = total_pages(&records); + assert!(files::write_v2_book_index( + &root, KEY, IDENTITY_A, pages, &records, &store, false, 0 + )); + assert!( + files::write_position_file(&root, KEY, 5, 42).is_ok(), + "write position for Book A" + ); + assert_eq!(files::read_position_file(&root, KEY), Some((5, 42))); + + // Book B adopts colliding KEY. + let adoption = files::adopt_layout_config(&root, KEY, IDENTITY_B, &store); + assert!(adoption.succeeded(), "Book B adoption must succeed"); + + assert_eq!( + files::read_position_file(&root, KEY), + None, + "Book A's saved position must be purged and not inherited by Book B" + ); + let book = files::open_v2_book_dir(&root, KEY).expect("book dir"); + assert!( + book.open_file_in_dir("POS.BIN", embedded_sdmmc::Mode::ReadOnly) + .is_err(), + "POS.BIN must be gone" + ); +}