From 7384a32cdd16ced7b7fce4dc3c712e52c69cba7a Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 8 Aug 2026 12:21:02 +0200 Subject: [PATCH] docs: add architecture review with findings R1-R8 Independent review of the codebase at c5c61a3. Records the architectural verdict and eight findings not covered by LAYOUT_REMEDIATION_SPEC.md, cross-referencing the existing spec where they overlap. Co-Authored-By: Claude Opus 5 --- docs/ARCHITECTURE_REVIEW.md | 507 ++++++++++++++++++++++++++++++++++++ 1 file changed, 507 insertions(+) create mode 100644 docs/ARCHITECTURE_REVIEW.md diff --git a/docs/ARCHITECTURE_REVIEW.md b/docs/ARCHITECTURE_REVIEW.md new file mode 100644 index 0000000..e52c260 --- /dev/null +++ b/docs/ARCHITECTURE_REVIEW.md @@ -0,0 +1,507 @@ +# Architecture Review + +Independent review of the codebase at `c5c61a3` (2026-08-06), answering one +question: **is this a well-architected library or an over-complex mess?** + +It is a well-architected library with one rotten subsystem inside it. The core +design holds up; roughly a fifth of the code is speculative or non-functional, +and it is concentrated in the ereader pagination/buffering layer. + +This document records the verdict, the evidence, and the findings **not already +covered** by [LAYOUT_REMEDIATION_SPEC.md](LAYOUT_REMEDIATION_SPEC.md). Where a +finding is already specced, it is cross-referenced rather than restated. + +## Contents + +| ID | Finding | Severity | Status | +|----|---------|----------|--------| +| [R1](#r1--the-process-pool-crashes-on-python-314) | The process pool crashes on Python 3.14 | Critical | New; raises priority of S12 | +| [R2](#r2--the-test-suite-hangs-at-interpreter-exit) | Test suite hangs at interpreter exit | High | New; same root cause as R1 | +| [R3](#r3--font-scaling-destroys-hyperlinks) | Font scaling destroys hyperlinks | High | New | +| [R4](#r4--declared-dependencies-are-incomplete) | Declared dependencies are incomplete | High | New | +| [R5](#r5--monkey-patched-page-methods-with-a-conflicting-signature) | Monkey-patched `Page` methods with a conflicting signature | Medium | New | +| [R6](#r6--dead-duck-typing-cluster-in-pagepy) | Dead duck-typing cluster in `page.py` | Low | New; extends S10.3 | +| [R7](#r7--two-orphaned-subsystems) | Two orphaned subsystems | Low | New | +| [R8](#r8--backward-pagination-is-guesswork) | Backward pagination is guesswork | Medium | Partially noted in S11 | + +--- + +## Verdict + +**Well architected.** The concerns that usually decide this question are all on +the right side of the line: + +- **The abstract/concrete split is real, not aspirational.** Verified + empirically: `abstract/` never imports `concrete/`; `core/` imports nothing but + itself. The single crossing ([document.py:7](../pyWebLayout/abstract/document.py#L7)) + is into `style/`, which the dependency rules permit. Most codebases claiming + this layering have leaked it within a year. +- **The layouter contract is the right abstraction.** `paragraph_layouter` + returning `(fit, failed_word_index, remaining_pretext)` is what makes + pagination resumable, and the shape is consistent across content types. Layout + engines that return `List[Line]` cannot paginate without a second pass. +- **[core/cache.py](../pyWebLayout/core/cache.py) is exemplary.** Usage-ranked + eviction with periodic aging, sampled eviction instead of a maintained heap, + O(1) hit path with no reordering — every choice justified by a measurement in + the docstring. +- **[concrete/text.py](../pyWebLayout/concrete/text.py) is the strongest file.** + The glyph cache ([:581-647](../pyWebLayout/concrete/text.py#L581-L647)) + reimplements PIL's internals to skip per-call setup, with a permanent graceful + fallback when the private API is absent. Alignment is a clean strategy pattern, + and `render_alignment_handler` handles last-line-of-paragraph correctly. +- **[html_extraction.py](../pyWebLayout/io/readers/html_extraction.py) is + textbook.** An immutable `StyleContext` threaded down the tree plus a handler + dispatch table — no giant if/elif, no mutable parser state. + +**The complexity that is not earned** is concentrated in three places, all in the +same band of the code: + +1. `layout/page_buffer.py` — 520 lines of multiprocess prefetch that has never + worked (S12, plus R1/R2 below). +2. `BidirectionalLayouter.render_page_backward` — ~100 lines of convergence + heuristics standing in for an anchor list that already exists (R8). +3. The second block dispatcher in `ereader_layout.py`, which silently drops + tables and lists (S4, S8). + +The pattern is visible in the git history: work from `e000068` onward (caching, +alignment, page geometry) is markedly better than the ereader scaffolding it sits +on. This is not a mess. It is a solid library with an early prototype still +embedded in it. + +**Sizing the cleanup:** R1–R7 plus S12 and S10.3 remove roughly 800–900 lines and +fix four user-visible defects. None of it requires redesigning anything. + +## Test baseline + +At `c5c61a3`, in a clean venv on Python 3.14.6: + +``` +833 passed, 2 skipped, 24 subtests passed in 11.51s +``` + +(Two further failures in `test_concrete_image.py` are environmental — the review +venv lacked `requests`. See R4, which is the same underlying problem.) + +The suite then **hangs indefinitely** rather than exiting. See R2. + +## Status of the existing remediation spec + +| Spec | Subject | State | +|------|---------|-------| +| S1 | Inline content in non-paragraph containers | Done (`284d521`) | +| S2 | Page geometry: origin and content rect | Done (`f18cec2`) | +| S3 | Draw/canvas lifecycle | Done (`202dacf`) | +| S11 | Partial-block progress discarded | Done (`a57da80`) | +| S13 | Word spacing and alignment | Done (`1262be6`) | +| S14 | Vertical centring in buttons and fields | Done (`c5c61a3`) | +| S4–S10, S12 | Dispatch, cells, table grid, pagination, hygiene, background rendering | Outstanding | + +The spec's analysis is sound and in places sharper than this review — S12 caught +that `_render_page_worker` omits `page_size` entirely, which this review missed. +Nothing below supersedes it. + +--- + +## R1 — The process pool crashes on Python 3.14 + +**Severity: critical. Raises S12 from "useless" to "fatal".** + +### Problem + +`PageBuffer` submits to a `ProcessPoolExecutor` from inside +`BufferedPageRenderer.render_page` +([page_buffer.py:431](../pyWebLayout/layout/page_buffer.py#L431)). Python 3.14 +changed the default multiprocessing start method on Linux from `fork` to +`forkserver`. Under a non-`fork` start method, `submit()` reaches +`_check_not_importing_main()`, which raises unless the caller sits inside an +`if __name__ == "__main__":` guard — and the child re-imports the caller's main +module, re-executing it. + +S12 documents this subsystem as delivering no benefit. On 3.14 it is worse than +that: `EreaderLayoutManager.get_current_page()` **raises** when called from +module-level script code. + +### Evidence + +A plain script calling `manager.get_current_page()` at module level, Python +3.14.6: + +``` +RuntimeError: + An attempt has been made to start a new process before the + current process has finished its bootstrapping phase. + ... + File "pyWebLayout/layout/page_buffer.py", line 221, in _queue_forward_renders + future = self.executor.submit(_render_page_worker, args) +ConnectionResetError: [Errno 104] Connection reset by peer +``` + +With a `__main__` guard added it does not raise, and instead confirms S12's +finding on every job: + +``` +Background render failed for position RenderingPosition(...): cannot pickle 'ImagingCore' object +Background render failed for position RenderingPosition(...): cannot pickle 'ImagingCore' object +``` + +### Action + +Fold into **S12**, and treat S12 as unblocked and urgent rather than phase 4. The +recommended resolution there — delete the pool, keep the LRU buffers and position +maps, replace prefetch with synchronous readahead — resolves R1 and R2 as a side +effect. S12's measurement gate still applies to the *readahead* decision; it does +not need to gate deletion of the pool, because the pool's contribution is +provably zero. + +### Files + +`pyWebLayout/layout/page_buffer.py` + +--- + +## R2 — The test suite hangs at interpreter exit + +**Severity: high.** Same root cause as R1. + +### Problem + +`PageBuffer.__del__` calls `shutdown()`, which calls +`executor.shutdown(wait=True)` ([page_buffer.py:342](../pyWebLayout/layout/page_buffer.py#L342)). +`EreaderLayoutManager.__del__` does the same via `renderer.shutdown()`. Running +`__del__` at interpreter shutdown and blocking on a process pool inside it +deadlocks. + +### Evidence + +``` +833 passed, 2 skipped, 24 subtests passed in 11.51s +``` + +...then the process sat at ~0% CPU with idle forkserver children for 13 minutes +before being killed. Reproduced twice; both runs completed the tests in under +12s and neither exited. + +This is why CI wall-clock does not resemble the 11.5s the tests actually take. + +### Action + +Resolved by S12's deletion of the executor. If for any reason the pool is +retained, `__del__` must not block: register an `atexit` handler or require +explicit `shutdown()`, and never `wait=True` from a finaliser. + +### Acceptance criteria + +- `pytest` returns to the shell within a second of printing its summary line. +- No `multiprocessing` child processes outlive the test session. + +### Files + +`pyWebLayout/layout/page_buffer.py`, `pyWebLayout/layout/ereader_manager.py` + +--- + +## R3 — Font scaling destroys hyperlinks + +**Severity: high. User-visible, silent, and trivially reproducible.** + +### Problem + +`BidirectionalLayouter._scale_block_fonts` +([ereader_layout.py:474-498](../pyWebLayout/layout/ereader_layout.py#L474-L498)) +rebuilds a scaled block by constructing plain `Word(word.text, scaled_style)` for +every word. `LinkedWord` is a `Word` subclass +([inline.py:288](../pyWebLayout/abstract/inline.py#L288)), so the reconstruction +downgrades it and the link target is discarded. + +The function returns the block unchanged when `font_scale == 1.0` and no family +override is set, which is why no test has caught this: the defect only appears +once the reader changes font size. + +Two further gaps in the same function: + +1. It handles only `Paragraph` and `Heading`. Every other block type is returned + unscaled, so a font-size change leaves images, tables and lists at their + original size while the text around them reflows. +2. It allocates a new `Paragraph` and a new `Word` per word on **every page + render** at any scale ≠ 1.0 — directly against the caching work in + `concrete/text.py`, and on the hot path. + +### Evidence + +At `c5c61a3`, parsing `

Go to this link now.

`: + +``` +scale=1.0: LinkedWords = 2 +scale=1.5: LinkedWords = 0 +``` + +### Design + +Stop reconstructing abstract blocks at layout time. Font scale and family are +*rendering context*, not document content — carrying them in a copied document +violates the "abstract content is not mutated by layout" principle in +[ARCHITECTURE.md](../ARCHITECTURE.md) in spirit, even though it copies rather +than mutates. + +Preferred: thread the scale/override into the layouter and resolve fonts at +`Text` construction, where `Font` objects are already deduplicated by +`FontRegistry`. `paragraph_layouter` already accepts an `alignment_override`; +`font_scale` and `font_family_override` belong in the same place. + +Minimum viable fix if the larger change is deferred: reconstruct via +`type(word)` and copy subclass state, and extend coverage to every block type. +This is strictly a stopgap — it keeps the per-page allocation cost. + +### Acceptance criteria + +- A document containing `` retains every `LinkedWord` after + `set_font_scale(1.5)`, and `query_point` over the rendered page still returns + `object_type="link"` with the correct target. +- An image block's rendered size is unaffected by `set_font_scale`, or scales + deliberately — not left inconsistent with the text around it. +- No new `Word`/`Paragraph` objects are allocated per page render at scale ≠ 1.0. + +### Files + +`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/document_layouter.py` + +--- + +## R4 — Declared dependencies are incomplete + +**Severity: high. A clean `pip install pyWebLayout` fails on first import.** + +### Problem + +[setup.cfg](../setup.cfg) declares: + +``` +install_requires = + Pillow + numpy +``` + +The library also imports `pyphen` ([abstract/inline.py](../pyWebLayout/abstract/inline.py)), +`bs4` ([io/readers/html_extraction.py](../pyWebLayout/io/readers/html_extraction.py)) +and `ebooklib` ([io/readers/epub_reader.py](../pyWebLayout/io/readers/epub_reader.py)). +`import pyWebLayout.concrete` fails without `pyphen`. + +There is no declared test extra either; `tests/concrete/test_concrete_image.py` +needs `requests`, which nothing declares. + +### Action + +- Add `pyphen`, `beautifulsoup4` and `ebooklib` to `install_requires`, with lower + bounds. +- Add an `[options.extras_require] test =` entry covering `pytest` and `requests`. +- Add a CI job that installs the built wheel into an empty environment and runs + `python -c "import pyWebLayout.concrete, pyWebLayout.io.readers.epub_reader"`. + This class of defect is only ever caught by installing what you ship. + +### Files + +`setup.cfg`, `setup.py`, CI configuration + +--- + +## R5 — Monkey-patched `Page` methods with a conflicting signature + +**Severity: medium.** Currently inert; a landmine if `Page` is ever refactored. + +### Problem + +[ereader_layout.py:741-761](../pyWebLayout/layout/ereader_layout.py#L741-L761) +defines `_add_page_methods()` and calls it at import time. It attaches +`can_fit_line` and `available_width` to the `Page` class if they are absent. + +`Page` defines both ([page.py:59](../pyWebLayout/concrete/page.py#L59), +[page.py:147](../pyWebLayout/concrete/page.py#L147)), so the patch never fires. +But the two definitions of `can_fit_line` **do not agree**: + +| Source | Signature | +|--------|-----------| +| `Page` | `can_fit_line(baseline_spacing, ascent=0, descent=0)` | +| monkey patch | `can_fit_line(line_height)` | + +The patched version also ignores descenders entirely — the exact bug S2 fixed. If +`Page.can_fit_line` were ever renamed or moved, this would silently reinstate +pre-S2 clipping behaviour, from an import side effect in a different package. + +### Action + +Delete `_add_page_methods` and its call site. Import-time monkey-patching of a +class in another module has no place here; if `Page` is missing something the +layout engine needs, it belongs on `Page`. + +### Files + +`pyWebLayout/layout/ereader_layout.py` + +--- + +## R6 — Dead duck-typing cluster in `page.py` + +**Severity: low.** Extends S10.3. + +### Problem + +[page.py:261-491](../pyWebLayout/concrete/page.py#L261-L491) contains a closed +cluster with no external callers: + +- `_get_child_property` (:261) — called only by the four below +- `_get_child_height` (:301) — called by nothing +- `_get_child_position` (:382) — called only by `_point_in_child` +- `_point_in_child` (:435) — called by nothing +- `_get_child_size` (:466) — called only by `_point_in_child` + +Verified by grep across `pyWebLayout/`, `tests/`, `examples/` and `scripts/`: +zero references outside the cluster. About 90 lines. + +It exists because `Renderable` declares neither `size` nor `origin`, so the code +probes `_size`, `size`, `_height`, `height`, `_origin` and `position` in turn with +`hasattr`. `query_point` (:399) already does the right thing instead — it relies +on the `Queriable` interface. + +### Action + +- Delete all five methods. +- Add `origin` and `size` to the `Renderable`/`Geometric` contract in + `core/base.py` so the duck-typing cannot grow back. This is the same concern as + S10.1's render contract and can ship with it. + +### Files + +`pyWebLayout/concrete/page.py`, `pyWebLayout/core/base.py` + +--- + +## R7 — Two orphaned subsystems + +**Severity: low**, but they are a large share of the "is this over-complex?" +impression: 559 lines that nothing in the library reaches. + +### Problem + +**`concrete/interaction_handler.py` (310 lines).** `InteractionHandler` and +`InteractionStateManager` are referenced only by +`examples/07_pressed_state_demo.py`. No library code, no ereader path, no tests. + +**`core/highlight.py` (249 lines).** `Highlight`, `HighlightColor` and +`HighlightManager` have tests (`tests/core/test_highlight.py`) but are not wired +into `EreaderLayoutManager` at all. Highlighting is not reachable through the +library's own top-level interface. + +`HighlightManager` also duplicates `BookmarkManager`'s JSON persistence +(directory, `_save`, `_load`, per-document file naming) with no shared base. + +### Action + +Decide per subsystem, and record the decision: + +- **Wire it up** — `EreaderLayoutManager` grows `add_highlight` / `highlights_for_page` + and the persistence merges with `BookmarkManager` into one document-state store. +- **Or move it out** — relocate to `examples/` or delete, and drop the tests with it. + +Either is fine. Leaving a tested, documented, unreachable subsystem in `core/` is +what makes the library look larger and less coherent than it is. + +### Files + +`pyWebLayout/concrete/interaction_handler.py`, `pyWebLayout/core/highlight.py`, +`pyWebLayout/layout/ereader_manager.py` + +--- + +## R8 — Backward pagination is guesswork + +**Severity: medium.** S11's closing note already flags this for audit; this +records what the audit found. + +### Problem + +`render_page_backward` +([ereader_layout.py:372-472](../pyWebLayout/layout/ereader_layout.py#L372-L472)) +finds the previous page by estimating a start position, rendering forward, +comparing the end against the target, and adjusting — **up to 10 times**. It then +has a fallback that jumps back up to 5 blocks and renders again, and a fallback +for *that* which renders from the start of the document. + +Worst case: one "previous page" tap costs up to 12 full page layouts. + +The estimator it converges from is `max(1, int(10 / font_scale))` blocks +([:684](../pyWebLayout/layout/ereader_layout.py#L684)) — a constant with no +relationship to page size, block length or font metrics. + +The correct answer is usually already known. +`EreaderLayoutManager._page_history` ([ereader_manager.py:210](../pyWebLayout/layout/ereader_manager.py#L210)) +records real page-start positions and serves them instantly; the refinement loop +only runs when history misses — after a jump, a bookmark, a font change, or +beyond 50 entries. + +S11's note asks whether these fallbacks were compensating for the +discarded-progress bug it fixed. They were, in part: the "failed to move +backward" branch at [:446](../pyWebLayout/layout/ereader_layout.py#L446) is +reachable precisely when forward rendering fails to advance, which S11 addressed. + +### Design + +Replace convergence with anchors. Maintain a sorted list of known page-start +positions — chapter starts from `ChapterNavigator` (free, already built) plus +every position visited. To go back from position P: binary-search the largest +anchor A < P, render forward from A collecting page starts until reaching P, and +return the last one. Cost is bounded by the anchor spacing, and every page start +discovered on the way is itself a new anchor, so the second traversal of any +region is free. + +This subsumes `_page_history`, so the two mechanisms become one. + +**Sequencing:** do this after S12, and after S8 — table and list pagination +changes what a page start can be, and re-deriving anchors is cheap only once +positions round-trip through tables correctly (S8 already notes this dependency). + +### Acceptance criteria + +- `previous_page()` from any position issues at most *k* page layouts, where *k* + is the anchor spacing, with no iteration count and no fallback ladder. +- Forward-then-backward round-trips exactly, from a cold cache, after a chapter + jump, and after a bookmark restore. +- `_estimate_page_start`, `_adjust_start_estimate` and the three-tier fallback + are deleted, not retained alongside. + +### Files + +`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/ereader_manager.py` + +--- + +## Recommended order + +``` +R4 ── packaging; independent, minutes, unblocks clean CI +S12 ── delete the process pool; resolves R1 and R2 with it +R3 ── font scaling loses links; independent, user-visible +R5 ── delete the monkey patch; minutes +R6 ── delete the dead cluster (with S10.1's render contract) +R7 ── decide the two orphans; no code risk either way +S4 → S5 → S6 → S7 → S8 → S9 (existing spec, unchanged) +R8 ── after S8 +``` + +R4, R5 and R6 are an afternoon and carry no design risk. S12 is the largest +single removal and fixes two defects at once. R3 is the one users would notice +today. Everything after that is the existing spec, which needs no revision. + +## Reproducing the findings + +Reviewed at `c5c61a3` on Python 3.14.6, in a venv containing +`pytest pyphen Pillow numpy beautifulsoup4 lxml ebooklib`. + +- **R1**: call `EreaderLayoutManager(...).get_current_page()` from module-level + script code (no `__main__` guard). +- **R2**: `python -m pytest -q`; observe the summary line, then the hang. +- **R3**: parse HTML containing ``, call + `BidirectionalLayouter._scale_block_fonts(block, 1.5)`, count `LinkedWord` + instances in the result. +- **R5**: compare `inspect.signature(Page.can_fit_line)` against the patch body. +- **R6**: grep the five method names across `pyWebLayout/ tests/ examples/ scripts/`. +- **R8**: read the loop; no execution needed.