Add EPUB and ZIM (Project Gutenberg) support - #20
Conversation
Update the `go` directive in go.mod (1.23.3 -> 1.26.5) and the builder image in the Dockerfile (golang:1.23 -> golang:1.26). This keeps the build environment aligned with the local toolchain and unblocks dependencies that require a newer Go. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bookworm previously indexed only .fb2 files and .zip archives of .fb2. This adds two new input formats: - .epub files: a new OPF/Dublin Core metadata reader (epub.go) extracts title, author (split via opf:file-as), language, subjects and cover. - .zim archives: enumerate the EPUB entries of a ZIM (e.g. a Kiwix Project Gutenberg library) via github.com/tim-st/go-zim, reading each book's metadata from its embedded EPUB (zim.go). One row per book, reopened on download by its namespace/URL reference. The download path (GetBook) is unified around an openSource helper that yields the raw bytes plus their source format. When the requested output format matches the source it is streamed as-is (so an EPUB source needs no conversion); otherwise convertBook shells out to ebook-convert, now generalized to any in/out format pair. GetBook always returns a callable cleanup, avoiding a nil-deref in the HTTP handler. The scan worker is refactored into per-format indexFB2/indexZip/ indexEPUB/indexZIM methods; the zip indexer now guards its shared slice with a mutex and closes entry readers. EPUB parsing is covered by unit + service tests using a synthetic epub. ZIM parsing is covered against a real Project Gutenberg archive committed in testdata (the smallest Kiwix Gutenberg ZIM that still contains a book, ~2 MB); both tests run unconditionally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
scripts/sync_gutenberg_en.py (stdlib only) reads the Kiwix Gutenberg directory listing and downloads the English archives you are missing: - English (gutenberg_en_*) only, newest version of each variant. - Skips archives already present locally. - Skips an lcc-* split when a same-or-newer -all archive is present (or will be downloaded this run), since a full -all already contains it; pass --include-superseded to fetch them anyway. - Resumable downloads (.part file + HTTP Range), which survives the mirror redirect. Supports --dir, --base-url, --dry-run, --include-superseded, --timeout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Run `go get -u ./...` + tidy + vendor. Notable direct/indirect bumps: mattn/go-sqlite3 1.14.24->1.14.49, rubenv/sql-migrate 1.7.1->1.8.1, golang.org/x/crypto 0.31->0.55, go-sql-driver/mysql 1.8.1->1.10.0, google.golang.org/protobuf 1.35.2->1.36.12. The `go` directive stays at 1.26.5 (no dependency requires newer). Build, vet, fmt and the non-conversion test suite pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dcd57e1c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| log.Printf("[service] storing %d books from zim %s\n", len(mds), path) | ||
|
|
||
| if len(mds) > 0 { | ||
| if err := s.store.Store(ctx, mds); err != nil { |
There was a problem hiding this comment.
When a ZIM contains more than one EPUB—as normal Gutenberg all and lcc-* archives do—this passes a multi-element slice to SqliteStore.Store. Its insertMetadatasBatch subsequently invokes NamedExec on FROM books WHERE file_path = :file_path with the same slice; because that query has no VALUES clause for sqlx to expand, SQLite receives one placeholder with multiple arguments and the transaction rolls back. The single-book Cornish fixture does not expose this, but the English archives targeted by the new sync script will index no books.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This doesn't reproduce — the multi-book path works.
I indexed a real multi-book Gutenberg archive (gutenberg_en_lcc-pf, 20 EPUBs all sharing one file_path) with the current deps: Store succeeds and all 20 rows land in both books and books_fts and are searchable by title (20/20, no rollback).
Mechanism: go-sqlite3 runs the second NamedExec through its connection Exec path, so the INSERT … SELECT … WHERE file_path = :file_path executes once and its SELECT fans out to every row of that archive — one FTS row per book. Verified there's no duplication either: 3 books → 3 FTS rows, 20 → 20. The duplicated identical :file_path args are harmless.
This is also the exact code the pre-existing multi-.fb2 ZIP ingestion has always used, and the full suite (incl. this batch path) is green in CI under Docker/Calibre. So no change here.
There was a problem hiding this comment.
Correction to my previous reply — you were right that there's a real multi-book defect here; I initially only exercised the ≤500-book path and missed it.
Store inserts in batches of 500, and the FTS population re-selected the whole archive (... FROM books WHERE file_path = :file_path) on every batch. So for an archive with >500 books (i.e. normal Gutenberg ZIMs) each batch re-indexed all earlier batches, duplicating FTS rows: e.g. 1200 books → 2700 books_fts rows, and a first-batch title matched 3× in search. The mechanism isn't a rollback / empty index, but the outcome (broken search for real Gutenberg archives) is real.
Fixed in 53d1995: the FTS insert now mirrors exactly the rows each batch inserted — a single multi-row INSERT assigns contiguous rowids ending at LastInsertId, so it selects that rowid range (WHERE id BETWEEN ? AND ?) instead of by file_path. Added a regression test that stores 1100 books in one archive and asserts books_fts == 1100 with each title found exactly once. Thanks for the catch.
| sys.stderr.write(msg) | ||
| sys.stderr.flush() | ||
| sys.stderr.write("\n") | ||
| os.replace(part, dst) |
There was a problem hiding this comment.
If the server closes a large download early while advertising a Content-Length, repeated HTTPResponse.read(size) calls can return EOF without raising an exception, and this unconditional rename promotes the truncated .part file to a completed .zim. Subsequent runs then skip it as already present, leaving a corrupt archive until it is manually deleted; compare downloaded with the expected total before calling os.replace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in faa65b4.
download() now compares the received byte count against the advertised Content-Length before os.replace; on a short read it raises and leaves the .part in place so the next run resumes, instead of promoting a truncated file to a finished .zim.
Verified with a local server that advertises 1000 bytes but sends 500 then closes: read() returns EOF without raising (exactly your scenario), the .part is kept at 500 B and no .zim is produced; a complete response still finalizes correctly.
If the server advertises a Content-Length but closes the connection early, urllib's read() returns EOF without raising, and the previous code renamed the short .part to the final .zim -- later runs then skip it as already present, leaving a corrupt archive. Verify the received byte count against the advertised total before os.replace; on a short read, raise and keep the .part so the next run resumes. Reported by Codex review on #20. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Store() inserts books in batches of 500. insertMetadatasBatch populated the FTS index with `INSERT INTO books_fts ... SELECT ... FROM books WHERE file_path = :file_path`, which re-selected the *entire* archive on every batch. For an archive with more than 500 books (normal Project Gutenberg ZIMs), each batch re-indexed all earlier batches, so a book in batch 1 was inserted into books_fts once per subsequent batch: e.g. 1200 books produced 2700 FTS rows and a first-batch title matched 3x in search. Insert FTS rows for exactly the rows each batch just inserted instead: a single multi-row INSERT assigns contiguous rowids ending at LastInsertId, so the batch spans [lastID-len+1, lastID]; select that rowid range. This is also independent of file_path and O(n) overall. Surfaced while re-examining the Codex review on #20 (which flagged the multi-book path, though the failure mode is duplication, not a rollback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d19955c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func (z *File) Close() { | ||
| z.f.Close() | ||
| } |
There was a problem hiding this comment.
For Zstandard-compressed archives, every zim.Open creates a zstd.Decoder, and reading a blob starts its worker goroutines; this Close only closes the underlying file, so each openZIMEntry request leaves the decoder, its workers, and their buffers alive. Repeated ZIM downloads therefore cause rapid, persistent goroutine and memory growth; close z.zstdReader here before closing the file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a56307. Correct: File.Close() closes only the os.File, and klauspost/compress/zstd leaks goroutines+memory unless the Decoder is Closed — go-zim never does. The decoder field is unexported (can't close it from our package) and upstream is gone (the repo now 404s; latest is v0.1.3 from 2021), so patching it isn't viable. Instead we reuse one *zim.File per path via a small cache, bounding it to one decoder per file for the process lifetime (we previously opened per download); access is serialized since *zim.File is not concurrency-safe.
(FWIW the other Go reader, Bornholm/go-zim, is worse on this axis: it creates a zstd.NewReader per blob read and never closes it.)
There was a problem hiding this comment.
Update: rather than keep working around this, we switched the ZIM reader from tim-st/go-zim to stazelabs/gozim (b05995f). It reads the same modern zstd archives but handles the decoder correctly — it pools decoders and uses the stateless DecodeAll, so there is no goroutine/memory leak — with the same pure-Go deps (klauspost/compress + xz) and no cgo. Verified against a real 20-book Gutenberg archive. The handle-reuse cache is gone; opening per request is fine now since nothing leaks.
| if err != nil { | ||
| log.Printf("[zim] error reading blob for %q in %q: %v", string(e.URL()), path, err) | ||
| continue |
There was a problem hiding this comment.
When a ZIM is incomplete or corrupt after at least one earlier EPUB remains readable, this silently skips the failed blob and eventually returns a nonempty slice with a nil error. indexZIM then stores that partial slice, and IsProcessed treats the archive as permanently complete, so later scans—including after an interrupted copy finishes—never index the missing books; propagate archive read failures instead of committing a partial archive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a56307, two ways:
- go-zim does not validate completeness, so before trusting a scan we compare the file size recorded in the ZIM header against the size on disk and reject a short/still-copying file. This deterministically catches a truncated archive, which otherwise enumerates a partial or empty set with no error (I verified end-truncation of the fixture produced no error until this check was added).
- Per-entry blob read/decompress errors are now propagated instead of skipped, so a corrupt archive fails rather than committing a partial list that
IsProcessedwould mark permanently complete. An individual EPUB whose OPF can't be parsed is still skipped so one malformed book doesn't block the whole archive.
Added a regression test that truncates the fixture and asserts the scan errors.
Two issues from the second Codex review on #20: 1. Decoder/goroutine leak: tim-st/go-zim's File.Close closes only the underlying os.File, never the zstd.Decoder that Open creates, and klauspost/compress/zstd leaks goroutines and memory unless every Decoder is Closed. We opened the ZIM on every download, so it leaked per request. The decoder field is unexported (can't be closed from our package) and upstream is gone (latest v0.1.3, 2021; repo 404), so patching it isn't viable. Reuse one *zim.File per path via a small cache instead, bounding it to one decoder per file for the process lifetime; access is serialized because *zim.File is not safe for concurrent use. 2. Partial/incomplete archive: a truncated or still-copying ZIM was enumerated without error, so indexZIM persisted a partial book list and IsProcessed then treated the archive as permanently complete. go-zim does not validate completeness, so compare the size recorded in the header against the size on disk and reject a short file, and propagate per-entry blob read errors. An individual EPUB whose metadata can't be parsed is still skipped so one bad book does not block the rest of the archive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mark vendor/** as linguist-vendored and linguist-generated so GitHub hides it by default in pull-request review and excludes it from language statistics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tim-st/go-zim leaked the zstd decoder on every archive open (its Close never closed the decoder) and its upstream repo has been removed from GitHub (latest v0.1.3, 2021). stazelabs/gozim reads the same modern zstd ZIMs but handles the decoder correctly -- it pools decoders and uses the stateless DecodeAll, so there is no goroutine/memory leak -- with the same pure-Go dependencies (klauspost/compress + xz) and no cgo. Verified against a real 20-book Project Gutenberg archive: all EPUBs enumerate and round-trip to valid blobs. Because the leak is gone at the source, drop the handle-reuse cache and go back to a plain open/close per operation (Service no longer carries a zimReader). The completeness check is kept but now reads the checksum position straight from the ZIM header, independent of the reader library, so a truncated/still-copying archive is still rejected rather than indexed partially. Stored SubFilepath refs (the "C/<url>" full path) are unchanged and match EntryByPath directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop an accidentally-committed scripts/__pycache__/*.pyc and add __pycache__/ and *.pyc to .gitignore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review Since the last review: fixed the FTS duplication for archives >500 books, and replaced the ZIM reader (tim-st/go-zim, which leaked the zstd decoder and whose repo is gone) with stazelabs/gozim, which pools decoders + uses DecodeAll (no leak). Also reject truncated/incomplete archives via a header size check. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b05995f983
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| headers["Range"] = "bytes=%d-" % have | ||
|
|
||
| req = urllib.request.Request(url, headers=headers) | ||
| with urllib.request.urlopen(req, timeout=timeout) as resp: |
There was a problem hiding this comment.
If the process is interrupted after writing the complete response but before os.replace, the next run sends Range: bytes=<file-size>-; a normal HTTP server responds with 416, and urlopen raises before this block can promote or restart the .part. The supposedly resumable download then fails on every subsequent run until the partial file is manually removed, so handle the EOF-range response by validating/promoting the complete partial or restarting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 16626bf. On a 416 with a .part present, download() now reads the total from the Content-Range: bytes */<total> header: if the .part already equals it (the "finished but not yet renamed" case), it's promoted with os.replace; otherwise the .part is discarded and the download restarts from scratch, so it self-heals instead of failing forever. Verified with a local server for three cases: complete .part→promoted, over-long .part→restarted, partial .part→normal 206 resume.
If the process is killed after the .part is fully written but before the os.replace, the next run requests Range: bytes=<full-size>- and the server answers 416 Range Not Satisfiable; urlopen raises and the download was stuck failing on every subsequent run until the .part was deleted by hand. Handle 416: read the total from the response's Content-Range and, if the .part already matches it, promote it; otherwise discard it and restart from scratch. Verified against a local server for the complete-part, over-long-part, and normal 206-resume cases. Reported by Codex review on #20. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16626bf6de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return nil, "", fmt.Errorf("error opening file %q in zip archive %q: %w", f.Name, zipPath, err) | ||
| } | ||
| defer rc.Close() | ||
| buf, err := io.ReadAll(rc) |
There was a problem hiding this comment.
When a client requests FB2 output for a book inside a ZIP, this now reads the entire uncompressed entry into memory before sending any response; the previous implementation returned the ZIP entry reader directly. Large FB2 entries—or several concurrent downloads—can therefore cause significant memory spikes or exhaust the server, so preserve streaming for same-format ZIP downloads (and only materialize input when conversion requires it).
Useful? React with 👍 / 👎.
Split the flat package main into internal/catalog (domain: types, Service, ServiceStore, SqliteStore, format parsers), internal/web and internal/telegram (thin frontends), leaving main.go as wiring. The frontends can now only reach catalog's exported API. SqliteStore stays in catalog so the Service<->store interface seam does not force an import cycle in the integration tests. Store: open two pools to the same file in WAL mode -- a single-connection write pool (serializes writers, no app mutex) and a multi-connection read pool -- so a long Store (indexing a whole ZIM) no longer blocks searches. Tests move from :memory: to a file-backed DB. Add Close(). ZIM: parse EPUB metadata on a bounded pool of NumCPU workers fed by a single serial blob reader. Measurement showed gozim serializes all decompression on one mutex while the EPUB parse (~91us/book) dominates and is lock-free, so this parallelizes the part that matters and caps peak memory to ~NumCPU blobs. convertBook: use os.CreateTemp instead of a hand-rolled nanosecond name. Fix a pre-existing data race in the fsnotify watcher: the AfterFunc callback deleted from debounceTimers on its own goroutine while the watcher goroutine touched the same map. Guard it with a mutex. Also carry the earlier review fixes: honor BIND_ADDR (was hardcoded :8080), reject FB2 with no parseable title instead of storing an empty row, and pass context into ebook-convert via CommandContext so a cancelled request kills the subprocess. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e64dc97fee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| remaining = resp.headers.get("Content-Length") | ||
| total = (have + int(remaining)) if remaining is not None else None |
There was a problem hiding this comment.
When a resumed request receives a 206 whose returned range does not extend to EOF, this treats the response segment's Content-Length as all remaining bytes. For example, Content-Range: bytes 4-7/10 with Content-Length: 4 makes total equal 8, so the short-read check passes and the 8-byte partial is renamed to .zim; subsequent syncs then skip the corrupt archive as present. The fresh evidence beyond the earlier short-read issue is that the final code still ignores the /10 resource length and starting offset advertised by Content-Range; parse and validate both before appending and publishing.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e64549638
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ): | ||
| yield "skip", arc, "covered by newer -all (%s)" % effective_all | ||
| continue | ||
| yield "download", arc, "missing" |
There was a problem hiding this comment.
When this script is rerun in the indexed books directory and a newer release exists for a locally present variant, have_date < arc.date falls through here and downloads the new filename while leaving the older .zim in place. Service.Scan indexes every .zim independently and the store has no cross-archive book deduplication, so each update duplicates essentially the whole variant in search results and grows the database; existing lcc-* files likewise remain duplicated after a covering all archive is downloaded. Retire or move superseded archives together with their indexed rows, or otherwise ensure only the effective archive set is indexed.
Useful? React with 👍 / 👎.
1 participant
Adds two new input formats to bookworm and a helper script for fetching Project Gutenberg libraries.
What's new
EPUB ingestion (
epub.go) — a new OPF / Dublin Core metadata reader extracts title, author (split viaopf:file-as), language, subjects and cover from.epubfiles. Previously EPUB was an output format only; bookworm now reads it as input too.ZIM ingestion (
zim.go) — enumerates theapplication/epub+zipentries of a ZIM archive (e.g. a Kiwix Project Gutenberg library) viagithub.com/tim-st/go-zim(pure Go, no cgo) and reads each book's metadata from its embedded EPUB. One row per book, reopened on download by its namespace/URL reference. PointBOOKS_DIRstraight at a.zim— no extraction step, no disk doubling.Unified download path —
GetBooknow streams the source as-is when the requested format already matches (so a Gutenberg EPUB needs no Calibre), and only shells out toebook-convertfor genuine.fb2↔.epubconversions.convertBookis generalized to any in/out pair, andGetBookalways returns a callable cleanup (fixes a latent nil-deref in the HTTP handler).Scan worker refactor — split into per-format
indexFB2/indexZip/indexEPUB/indexZIM; the zip indexer now guards its shared slice with a mutex and closes entry readers.Sync script (
scripts/sync_gutenberg_en.py, stdlib only) — downloads the English Gutenberg archives you're missing from the Kiwix mirror: English-only, newest version per variant, skips what's present, and skipslcc-*splits already covered by a same-or-newer-all. Resumable (.part+ HTTP Range, survives the mirror redirect).--dry-run,--include-superseded,--jobs-free by design.Commits
Bump Go to 1.26 to match local toolchain—go.mod+Dockerfile(a new dep requires Go ≥ 1.24).Add EPUB and ZIM (Project Gutenberg) support— the feature, plus tests.Add script to sync English Gutenberg ZIM archives from Kiwix.Testing
testdata/(the smallest Kiwix Gutenberg ZIM that still contains a book, ~2 MB). Both ZIM tests run unconditionally.gutenberg_en_lcc-pfZIM (20 books indexed, searched and served); metadata came out clean (titles,opf:file-asauthor splits, languages, subjects, covers).go build/go vetclean. The full suite runs in CI viamake docker-test(Calibre present); locally the pre-existing conversion tests needebook-convert, but the new EPUB/ZIM passthrough tests don't.🤖 Generated with Claude Code