feat(wdow): stream LLM-scan PDF render path, remove 200MB size gate #1015

Merged

zombor merged 2 commits from bd-bookshelf-wdow into main 2026-07-08 11:06:59 +00:00

Conversation 2 Commits 2 Files changed 10 +548 -105

zombor commented 2026-07-08 02:11:58 +00:00

Owner

Copy link

Summary

• Remove size gate: eliminates the info.Size() > MaxPDFBytes hard-reject (PR #1010 sibling) from LLMScanActivities.Scan; PDFs of any size are now accepted
• Stream the render path: adds RenderPDFPageImagesFromReader(ctx, io.ReadSeeker, int64, firstN, lastN int) in internal/files using pdfium's FPDF_LoadCustomDocument (demand-driven I/O); the wfengine adapter opens the file with os.Open → passes the *os.File (satisfies io.ReadSeeker) — no os.ReadFile / ReadAll
• Transient-vs-permanent classification: trackingReadSeeker wraps the reader; if OpenDocument fails AND a Read error was captured → retryable plain error; otherwise ErrNoCover (permanent). The wfengine's Scan activity classifies ErrNoCover as permanent (no retry), plain I/O errors as retryable — mirroring the fix in bookshelf-qwvg (#1010)

Test plan

• RenderPDFPageImagesFromReader: valid PDF → images
• Corrupt PDF → ErrNoCover (permanent)
• Zero-page PDF → ErrNoCover
• Transient Read error → plain error, does NOT wrap ErrNoCover
• All pages fail to render → ErrNoCover (exercises renderErr != nil + len(imgs) == 0 branches)
• Pool error, instance error, deadline exceeded
• Bytes-read proof: 10-page PDF, render pages [0,1,8,9], assert bytesRead < fileSize/2
• wfengine: openFile fails → retryable; renderPageImages returns ErrNoCover → permanent; returns transient I/O error → non-permanent
• make coverage → 100% on all affected packages
• make lint → 0 issues in modified packages

Closes bead bookshelf-wdow on merge.

## Summary - **Remove size gate**: eliminates the `info.Size() > MaxPDFBytes` hard-reject (PR #1010 sibling) from `LLMScanActivities.Scan`; PDFs of any size are now accepted - **Stream the render path**: adds `RenderPDFPageImagesFromReader(ctx, io.ReadSeeker, int64, firstN, lastN int)` in `internal/files` using pdfium's `FPDF_LoadCustomDocument` (demand-driven I/O); the wfengine adapter opens the file with `os.Open` → passes the `*os.File` (satisfies `io.ReadSeeker`) — no `os.ReadFile` / `ReadAll` - **Transient-vs-permanent classification**: `trackingReadSeeker` wraps the reader; if `OpenDocument` fails AND a `Read` error was captured → retryable plain error; otherwise `ErrNoCover` (permanent). The wfengine's `Scan` activity classifies `ErrNoCover` as permanent (no retry), plain I/O errors as retryable — mirroring the fix in bookshelf-qwvg (#1010) ## Test plan - [x] `RenderPDFPageImagesFromReader`: valid PDF → images - [x] Corrupt PDF → `ErrNoCover` (permanent) - [x] Zero-page PDF → `ErrNoCover` - [x] Transient `Read` error → plain error, does NOT wrap `ErrNoCover` - [x] All pages fail to render → `ErrNoCover` (exercises `renderErr != nil` + `len(imgs) == 0` branches) - [x] Pool error, instance error, deadline exceeded - [x] Bytes-read proof: 10-page PDF, render pages [0,1,8,9], assert `bytesRead < fileSize/2` - [x] wfengine: `openFile` fails → retryable; `renderPageImages` returns `ErrNoCover` → permanent; returns transient I/O error → non-permanent - [x] `make coverage` → 100% on all affected packages - [x] `make lint` → 0 issues in modified packages Closes bead bookshelf-wdow on merge.

zombor added 1 commit 2026-07-08 02:11:58 +00:00

feat(wdow): stream LLM-scan PDF render path, remove 200MB size gate ... 4dd4e17b0d

- Add RenderPDFPageImagesFromReader in internal/files taking io.ReadSeeker+size,
  using FPDF_LoadCustomDocument for demand-driven I/O (no ReadAll)
- Wrap reader in trackingReadSeeker: if OpenDocument fails AND a read I/O error
  was captured → return retryable plain error; otherwise ErrNoCover (permanent)
- Remove info.Size() > files.MaxPDFBytes hard-reject from LLMScanActivities.Scan
- Replace StatFile+ReadFile deps with unified OpenFile func(path) (io.ReadSeekCloser, int64, error)
- wfengine adapter classifies ErrNoCover from renderPageImages as permanent,
  plain I/O error as transient/retryable
- 100% coverage: streaming render, corrupt PDF, zero-page, transient I/O error,
  all-pages-fail (ErrNoCover), pool error, instance error, deadline, bytes-read proof
  (pdfium reads <50% of file bytes when rendering sampled pages of a 10-page PDF)

Closes bead bookshelf-wdow on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-07-08 02:23:49 +00:00

fix(wdow): update engine_integration_test to use OpenFile+new RenderPageImages sig ... 768d157a7b

engine_integration_test.go missed the StatFile/ReadFile → OpenFile migration
and still referenced the old RenderPageImages ([]byte) signature. Remove
the now-unused io/fs import.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-08 02:36:34 +00:00

Author

Owner

Copy link

Code Review — bookshelf-wdow / PR #1015

Reviewed: diff only (CI green, per policy review agents do not re-run tests).

---

Phase 0: DEMO Verification

No DEMO: block was posted in the bead comments — only Completed: and AWAITING REVIEW. For a bug-fix of this shape (removing a size gate + streaming), the natural DEMO would be "scan a 251 MB PDF successfully" with a before/after command. That was not provided. CI green is accepted as the effective DEMO for this session; flagged as a minor below.

---

Phase 1 & 2: Spec compliance + Code quality

What was checked (mirror of the #1010 lesson list):

1. Streaming render correctness — renderPDFPageImagesOnInstFromReader opens via OpenDocument{FileReader: tracker, FileReaderSize: size}, renders all sampled indices before the defer FPDF_CloseDocument, uses scanRenderWidth/scanRenderHeight, buildPageIndicesN dedup, and pdfScanRenderTimeout. All correct.

2. Transient-vs-permanent split — trackingReadSeeker (defined in cover_extract.go, confirmed excludes io.EOF) captures non-EOF read errors. In renderPDFPageImagesOnInstFromReader: tracker.readErr != nil → return bare I/O error (retryable); pdfium-rejects-content → errors.Join(err, ErrNoCover) (permanent). FPDF_GetPageCount and zero-page cases → ErrNoCover (permanent). All-pages-fail → ErrNoCover (permanent). In the wfengine adapter (Scan): errors.Is(err, files.ErrNoCover) → NewPermanentError; otherwise plain return (retryable). Architecture boundary preserved — internal/files does not import the workflow engine.

3. Memory-bound proof — makeScanMultiPagePDF places large noisy 200×300 JPEGs on pages 2–7, tiny 4×4 JPEGs on pages 0,1,8,9; buildPageIndicesN(10,2,2) selects [0,1,8,9]; scanCountingReadSeeker counts atomic-safe bytes; assertion is bytesRead < fileSize/2. Non-tautological and quantitatively robust.

4. openFile dep — os.Open + f.Stat(), returns (ReadSeekCloser, size, error). f.Close() called on stat error in build_extended_deps.go. defer reader.Close() in Scan. No resource leak.

5. Removing MaxPDFBytes gate — remaining bounds (WASM memory cap, pdfScanRenderDeadline, raster cap at scanRenderWidth×scanRenderHeight, maxRenderPages) still bound an adversarial PDF. Consistent with the #1010 rationale.

6. Black-box tests — package files_test, package wfengine_test throughout. No white-box test files added.

7. No new .golangci.yml exclusions added.

8. engine_integration_test.go updated — still exercises real New()+StartWorker with the new OpenFile stub, proving activity registration survives the signature change.

---

Findings

[MINOR] internal/files/pdf_page_images_streaming_test.go:292–297 — multiple Expect calls inside a loop in one It block
The It("returns decodable PNG images") block iterates over result and calls Expect(decErr).NotTo(HaveOccurred()) for each image. Project convention: exactly ONE Expect per It. Fix: assert Expect(result[0]).To(...) on a single representative image, or split into It("all images decode without error", func() { Expect(allDecodable(result)).To(BeTrue()) }) with a helper.

[MINOR] internal/wfengine/llm_scan_workflow_test.go:886–889 — vacuous assertion in "does not classify transient I/O error as permanent"
The block reads if errors.As(err, &wfErr) { Expect(wfErr.Permanent).To(BeFalse()) }. When the implementation is correct (plain error, no *gowf.Error wrapper), errors.As returns false and the Expect body never executes — the test is a no-op in the passing case and provides zero regression protection. Fix: Expect(errors.As(err, &wfErr)).To(BeFalse()) which positively asserts the error is never wrapped as a workflow error.

[MINOR] No DEMO block in bead comments
The AWAITING REVIEW comment contains no executable before/after command. For a size-gate removal of this kind the expected DEMO is a command that scans a >200 MB PDF and shows the former error is gone. Post a DEMO in a follow-up bead comment or accept CI green as sufficient (orchestrator decision).

---

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## Code Review — bookshelf-wdow / PR #1015 **Reviewed:** diff only (CI green, per policy review agents do not re-run tests). --- ### Phase 0: DEMO Verification No `DEMO:` block was posted in the bead comments — only `Completed:` and `AWAITING REVIEW`. For a bug-fix of this shape (removing a size gate + streaming), the natural DEMO would be "scan a 251 MB PDF successfully" with a before/after command. That was not provided. CI green is accepted as the effective DEMO for this session; flagged as a minor below. --- ### Phase 1 & 2: Spec compliance + Code quality **What was checked (mirror of the #1010 lesson list):** 1. Streaming render correctness — `renderPDFPageImagesOnInstFromReader` opens via `OpenDocument{FileReader: tracker, FileReaderSize: size}`, renders all sampled indices before the `defer FPDF_CloseDocument`, uses `scanRenderWidth`/`scanRenderHeight`, `buildPageIndicesN` dedup, and `pdfScanRenderTimeout`. All correct. 2. Transient-vs-permanent split — `trackingReadSeeker` (defined in `cover_extract.go`, confirmed excludes `io.EOF`) captures non-EOF read errors. In `renderPDFPageImagesOnInstFromReader`: `tracker.readErr != nil` → return bare I/O error (retryable); pdfium-rejects-content → `errors.Join(err, ErrNoCover)` (permanent). `FPDF_GetPageCount` and zero-page cases → `ErrNoCover` (permanent). All-pages-fail → `ErrNoCover` (permanent). In the wfengine adapter (`Scan`): `errors.Is(err, files.ErrNoCover)` → `NewPermanentError`; otherwise plain return (retryable). Architecture boundary preserved — `internal/files` does not import the workflow engine. 3. Memory-bound proof — `makeScanMultiPagePDF` places large noisy 200×300 JPEGs on pages 2–7, tiny 4×4 JPEGs on pages 0,1,8,9; `buildPageIndicesN(10,2,2)` selects [0,1,8,9]; `scanCountingReadSeeker` counts `atomic`-safe bytes; assertion is `bytesRead < fileSize/2`. Non-tautological and quantitatively robust. 4. `openFile` dep — `os.Open` + `f.Stat()`, returns `(ReadSeekCloser, size, error)`. `f.Close()` called on stat error in `build_extended_deps.go`. `defer reader.Close()` in `Scan`. No resource leak. 5. Removing `MaxPDFBytes` gate — remaining bounds (WASM memory cap, `pdfScanRenderDeadline`, raster cap at `scanRenderWidth×scanRenderHeight`, `maxRenderPages`) still bound an adversarial PDF. Consistent with the #1010 rationale. 6. Black-box tests — `package files_test`, `package wfengine_test` throughout. No white-box test files added. 7. No new `.golangci.yml` exclusions added. 8. `engine_integration_test.go` updated — still exercises real `New()+StartWorker` with the new `OpenFile` stub, proving activity registration survives the signature change. --- ### Findings [MINOR] internal/files/pdf_page_images_streaming_test.go:292–297 — multiple `Expect` calls inside a loop in one `It` block The `It("returns decodable PNG images")` block iterates over `result` and calls `Expect(decErr).NotTo(HaveOccurred())` for each image. Project convention: exactly ONE `Expect` per `It`. Fix: assert `Expect(result[0]).To(...)` on a single representative image, or split into `It("all images decode without error", func() { Expect(allDecodable(result)).To(BeTrue()) })` with a helper. [MINOR] internal/wfengine/llm_scan_workflow_test.go:886–889 — vacuous assertion in "does not classify transient I/O error as permanent" The block reads `if errors.As(err, &wfErr) { Expect(wfErr.Permanent).To(BeFalse()) }`. When the implementation is correct (plain error, no `*gowf.Error` wrapper), `errors.As` returns false and the `Expect` body never executes — the test is a no-op in the passing case and provides zero regression protection. Fix: `Expect(errors.As(err, &wfErr)).To(BeFalse())` which positively asserts the error is never wrapped as a workflow error. [MINOR] No DEMO block in bead comments The `AWAITING REVIEW` comment contains no executable before/after command. For a size-gate removal of this kind the expected DEMO is a command that scans a >200 MB PDF and shows the former error is gone. Post a DEMO in a follow-up bead comment or accept CI green as sufficient (orchestrator decision). --- REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor commented 2026-07-08 02:37:29 +00:00

Author

Owner

Copy link

Security Review — PR #1015 (bookshelf-wdow): PDF streaming scan, MaxPDFBytes gate removal

Central question: is peak memory bounded without the 200 MB gate?

YES — the removal is correct and safe for the streaming path. Detailed analysis below.

Memory bounding

The old 200 MB gate existed to prevent os.ReadFile from buffering an arbitrarily large PDF into Go heap. The new FPDF_LoadCustomDocument path eliminates the Go heap buffer entirely — pdfium calls back into trackingReadSeeker.Read for only the byte ranges it needs (cross-reference table, selected page object streams). No whole-file buffer is allocated in Go.

Three independent bounds now cover peak memory:

1. WASM linear memory cap — pdfiumMaxMemoryPages = 16384 pages × 64 KB = 1 GB hard cap (wazero.NewRuntimeConfig().WithMemoryLimitPages). Any decompression-bomb expansion of compressed xref/object streams inside pdfium is absorbed here; the cap will kill the WASM module before it OOMs the worker process.

2. Fixed scan raster per page — scanRenderWidth = 667, scanRenderHeight = 1000 → ~2.67 MB RGBA per page in WASM. Pages are rendered sequentially inside renderPDFPageImagesOnInstFromReader; resp.Cleanup() is called after each page is encoded to PNG, releasing the WASM raster allocation before the next page begins. Peak WASM ≈ one page decode at a time.

3. maxRenderPages = 5 cap — buildPageIndicesN returns at most 5 indices regardless of document length. The accumulated PNG byte slices in Go heap are therefore bounded at 5 × ~2.67 MB ≈ 13 MB uncompressed, which compresses to far less. The bytes-read proof test (RenderPDFPageImagesFromReader: bytes-read proof) empirically confirms on-demand I/O: 4 selected pages from a 10-page PDF consume < half the total file bytes.

CPU bounding

pdfScanRenderDeadline = 180 s with WithCloseOnContextDone(true) in the WASM runtime config. When the context deadline fires, pdfRenderSeams.closeInstance(inst) cancels the pdfium WASM context, interrupting the in-flight render. The <-ch drain (line 122) ensures the goroutine exits before RenderPDFPageImagesFromReader returns, so no goroutine leaks from the deadline branch.

Path traversal

openFile(fullPath) where fullPath is the output of scanSafeJoin(libraryRoot, subPath). Both libraryRoot (from getLibraryPath) and subPath (from getPDFSubPath) come from trusted DB records, not from HTTP request input. scanSafeJoin validates filepath.Clean(full)+sep has cleanRoot+sep as a prefix before returning. The //nolint:gosec on os.Open(path) is correctly justified by this prior validation.

File descriptor lifecycle

• os.Open error path: returns nil, 0, err — no fd allocated, no Close needed. ✓
• f.Stat() error path: f.Close() called in the error branch before returning. ✓
• Success path: Scan() registers defer reader.Close() immediately after the nil-error check; the file handle stays alive for the duration of renderPageImages (which holds a callback reference through trackingReadSeeker) and is closed on Scan return. ✓
• Deadline path: closeInstance(inst) interrupts WASM → goroutine unblocks → <-ch drains → Scan returns → defer reader.Close() fires. No fd leak. ✓

Error retryability

openFile failures (all errors including os.ErrNotExist) are returned as retryable, whereas the old readPDF wrapped all stat/read failures as gowf.NewPermanentError. This is a behavioral change, but it is moot in practice: llmInteractiveActivityOptions sets MaxAttempts: 1, so the Scan activity never retries regardless of error classification. The interactive caller surfaces failures to the user for manual retry.

The new transient/permanent split for renderPageImages errors (retryable on non-ErrNoCover, permanent on ErrNoCover) is correct.

Architecture boundary

internal/files/pdf_page_images.go imports only stdlib + pdfium packages — no wfengine import. The files.ErrNoCover → gowf.NewPermanentError mapping lives solely in internal/wfengine/llm_scan_workflow.go (the one allowed layer). ✓

Secrets / PII

No secrets or PII logged. The new "llm scan page images rendered" log line records book_id, page_count, and size_bytes_per_page (PNG sizes, not content). ✓

Test hygiene

All new test files declare package files_test / package wfengine_test (black-box). ✓

---

[MINOR] internal/files/pdf_page_images_streaming_test.go:292 — multi-Expect loop in unit test It block
It("returns decodable PNG images") contains a for loop with one Expect(decErr) per iteration, violating the strict one-Expect-per-It rule for unit tests. For a collection assertion, replace with a Gomega collection matcher such as Expect(result).To(HaveEach(Satisfy(func(b []byte) bool { _, err := png.Decode(bytes.NewReader(b)); return err == nil }))) or split into a parameterised DescribeTable. Single-page PDFs produce one element so this is currently harmless, but multi-page cases would silently produce multiple assertions under one It.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — PR #1015 (bookshelf-wdow): PDF streaming scan, MaxPDFBytes gate removal ### Central question: is peak memory bounded without the 200 MB gate? **YES — the removal is correct and safe for the streaming path.** Detailed analysis below. #### Memory bounding The old 200 MB gate existed to prevent `os.ReadFile` from buffering an arbitrarily large PDF into Go heap. The new `FPDF_LoadCustomDocument` path eliminates the Go heap buffer entirely — pdfium calls back into `trackingReadSeeker.Read` for only the byte ranges it needs (cross-reference table, selected page object streams). No whole-file buffer is allocated in Go. Three independent bounds now cover peak memory: 1. **WASM linear memory cap** — `pdfiumMaxMemoryPages = 16384` pages × 64 KB = **1 GB hard cap** (`wazero.NewRuntimeConfig().WithMemoryLimitPages`). Any decompression-bomb expansion of compressed xref/object streams inside pdfium is absorbed here; the cap will kill the WASM module before it OOMs the worker process. 2. **Fixed scan raster per page** — `scanRenderWidth = 667`, `scanRenderHeight = 1000` → ~2.67 MB RGBA per page in WASM. Pages are rendered **sequentially** inside `renderPDFPageImagesOnInstFromReader`; `resp.Cleanup()` is called after each page is encoded to PNG, releasing the WASM raster allocation before the next page begins. Peak WASM ≈ one page decode at a time. 3. **`maxRenderPages = 5` cap** — `buildPageIndicesN` returns at most 5 indices regardless of document length. The accumulated PNG byte slices in Go heap are therefore bounded at 5 × ~2.67 MB ≈ 13 MB uncompressed, which compresses to far less. The bytes-read proof test (`RenderPDFPageImagesFromReader: bytes-read proof`) empirically confirms on-demand I/O: 4 selected pages from a 10-page PDF consume < half the total file bytes. #### CPU bounding `pdfScanRenderDeadline = 180 s` with `WithCloseOnContextDone(true)` in the WASM runtime config. When the context deadline fires, `pdfRenderSeams.closeInstance(inst)` cancels the pdfium WASM context, interrupting the in-flight render. The `<-ch` drain (line 122) ensures the goroutine exits before `RenderPDFPageImagesFromReader` returns, so no goroutine leaks from the deadline branch. #### Path traversal `openFile(fullPath)` where `fullPath` is the output of `scanSafeJoin(libraryRoot, subPath)`. Both `libraryRoot` (from `getLibraryPath`) and `subPath` (from `getPDFSubPath`) come from trusted DB records, not from HTTP request input. `scanSafeJoin` validates `filepath.Clean(full)+sep` has `cleanRoot+sep` as a prefix before returning. The `//nolint:gosec` on `os.Open(path)` is correctly justified by this prior validation. #### File descriptor lifecycle - `os.Open` error path: returns `nil, 0, err` — no fd allocated, no Close needed. ✓ - `f.Stat()` error path: `f.Close()` called in the error branch before returning. ✓ - Success path: `Scan()` registers `defer reader.Close()` immediately after the nil-error check; the file handle stays alive for the duration of `renderPageImages` (which holds a callback reference through `trackingReadSeeker`) and is closed on `Scan` return. ✓ - Deadline path: `closeInstance(inst)` interrupts WASM → goroutine unblocks → `<-ch` drains → `Scan` returns → `defer reader.Close()` fires. No fd leak. ✓ #### Error retryability `openFile` failures (all errors including `os.ErrNotExist`) are returned as retryable, whereas the old `readPDF` wrapped all stat/read failures as `gowf.NewPermanentError`. This is a behavioral change, but it is **moot in practice**: `llmInteractiveActivityOptions` sets `MaxAttempts: 1`, so the Scan activity never retries regardless of error classification. The interactive caller surfaces failures to the user for manual retry. The new transient/permanent split for `renderPageImages` errors (retryable on non-ErrNoCover, permanent on ErrNoCover) is correct. #### Architecture boundary `internal/files/pdf_page_images.go` imports only stdlib + pdfium packages — no wfengine import. The `files.ErrNoCover` → `gowf.NewPermanentError` mapping lives solely in `internal/wfengine/llm_scan_workflow.go` (the one allowed layer). ✓ #### Secrets / PII No secrets or PII logged. The new `"llm scan page images rendered"` log line records `book_id`, `page_count`, and `size_bytes_per_page` (PNG sizes, not content). ✓ #### Test hygiene All new test files declare `package files_test` / `package wfengine_test` (black-box). ✓ --- [MINOR] internal/files/pdf_page_images_streaming_test.go:292 — multi-Expect loop in unit test `It` block `It("returns decodable PNG images")` contains a `for` loop with one `Expect(decErr)` per iteration, violating the strict one-Expect-per-It rule for unit tests. For a collection assertion, replace with a Gomega collection matcher such as `Expect(result).To(HaveEach(Satisfy(func(b []byte) bool { _, err := png.Decode(bytes.NewReader(b)); return err == nil })))` or split into a parameterised `DescribeTable`. Single-page PDFs produce one element so this is currently harmless, but multi-page cases would silently produce multiple assertions under one `It`. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-wdow from 768d157a7b to fa8e69a7a7 2026-07-08 11:01:56 +00:00 Compare

zombor merged commit 0722ba054c into main 2026-07-08 11:06:59 +00:00

zombor referenced this pull request from a commit 2026-07-08 11:07:02 +00:00

Merge pull request 'feat(wdow): stream LLM-scan PDF render path, remove 200MB size gate' (#1015) from bd-bookshelf-wdow into main

zombor referenced this pull request 2026-08-08 02:34:32 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa) #1397

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1015

No description provided.