fix(page-count): recover pages when /Count-based counting returns 0

[ajbufort]

Problem

When a document's page tree is packed inside an object stream (PDF 1.5+ routinely compresses the Catalog and /Pages nodes), or is a deeply nested /Pages -> /Pages -> /Page tree, both page_count() paths fail: get_page_count_standard() cannot resolve /Pages to a dictionary and returns Ok(0), and the manual get_page_count_by_scanning() fallback returns 0 on the same nodes. Because the result is Ok(0) (not an error), callers treat the document as empty and extract nothing.

get_page(i) meanwhile resolves every page fine — it uses its own per-page traversal / collect_all_pages bulk walk. So the pages are fully reachable; only the count is wrong. (Note all_page_refs is not a usable fallback here: it walks /Pages -> /Kids via collect_page_refs, which misses the same object-stream-packed tree.)

Concretely: a 14-page paper with its /Pages root in an /ObjStm reports page_count() == 0 and produces no output, though get_page(0..14) each return a valid page.

Fix

When the /Count-based readers yield 0 on a non-encrypted document, count by probing get_page — the definitive agreement with the pages the rest of the API can actually reach:

if matches!(primary, Ok(0)) && !self.is_encrypted() {
    let mut n = 0usize;
    while n < 1_000_000 && self.get_page(n).is_ok() { n += 1; }
    if n > 0 { return Ok(n); }
}

get_page never calls back into page_count (no recursion), caches each page (cheap repeat probes), and the loop is bounded. Gated on a primary count of 0, so every normally-counted document returns the exact same value as before — genuinely empty documents (where get_page(0) errors) still report 0.

Full gate on this branch off main: cargo test --lib (5750 passed), cargo test --doc (141 passed), cargo clippy --lib clean, cargo fmt --check clean.

[yfedoseev]

Thanks for this — the approach is sound and I appreciate that it's careful about blast radius.

What's good. The trigger guard is tight: the rescue block only runs when primary == Ok(0) && !self.is_encrypted(), so any document whose /Count reader returns > 0 skips it entirely and is byte-identical — well-formed PDFs are untouched. No new recursion, the walk terminates (real cycle detection via the visited set, plus the < 1_000_000 belt-and-suspenders bound), and it delegates to get_page/the existing recover_from_object_streams machinery rather than duplicating recovery logic. Nicely DRY.

Blocker — please add a test. A change to page discovery needs a regression test, and this PR ships none. Please add a crafted fixture that reproduces the motivating case — an ObjStm-packed /Pages (or otherwise broken /Count) document where page_count() returned 0 — and assert it now returns the real N. Please confirm it fails on main and passes with your change (revert the document.rs hunk to check). That's the repo's bar for recovery fixes.

Minor (non-blocking):

1. The outer comment references all_page_refs walking /Pages→/Kids "so agree with it", but the whole premise is that all_page_refs/collect_page_refs miss these trees and get_page succeeds via its own traversal. The wording is misleading — please tighten it to match the inner comment.
2. In the ObjStm-packed case each get_page(n) can fall back to get_page_by_scanning (O(objects) per call) → O(n·objects) to count. Bounded and only on already-broken docs, but worth a one-line acknowledgement for very large recovered files.

I ran a 419-corpus regression to confirm no change on well-formed PDFs (the guard should guarantee it) — will post the result. Once the test lands this should be good to go.

[yfedoseev]

Corpus regression result (419-PDF suite, main vs this PR): 0 differing files across text/markdown/html.

That's the expected and desired outcome — the primary == Ok(0) guard means every well-formed PDF (all 419 here have a valid /Count) skips the rescue block and is byte-identical. So this confirms no regression on well-formed documents. It can't demonstrate the gain because the suite has no broken-/Count/ObjStm-packed docs — which is exactly why the crafted fixture test (asserting page_count() goes 0 → N) is the remaining ask.

[yfedoseev]

Hi @ajbufort — first, a real thank-you. Your PRs target genuine, well-diagnosed gaps (inline-image decode, tagged-table reading order, deterministic font subsets, catalog recovery, off-MediaBox culling, and more), and the code quality is high. Several are merged or queued. We want to keep merging your work — this note is about making that faster and more reliable, not slower.

Where we are. You currently have 9 open PRs, each a behavioral change to the extraction core. Two consequences:

• CI capacity: every push runs our full multi-language matrix (~215 checks — Python/Go/Node/C#/Java/WASM/Ruby bindings, wheels, FIPS, 3 OSes). The runners realistically process one PR's matrix at a time, so 9 concurrent PRs saturate the queue and slow everyone's — including yours.
• Verification load: because these change extraction output, each needs a full corpus regression before it's safe to merge, and today we're the first ones running that. That's a lot of maintainer time per PR, and it's the main thing gating your merges.

What we're now asking on every behavioral PR (this is being formalized in our contribution policy, #901 — CONTRIBUTING.md):

1. An accepted issue first. Before the code, open an issue: the problem, a minimal PDF you built yourself (or a description of the structure — please don't attach copyrighted/third-party PDFs; they can't be fixtures), the relevant ISO 32000 clause, and the expected before→after. Non-trivial PRs must reference an issue we've accepted (Closes #NNN). This gets us agreeing on scope up front and gives each PR a clear definition of done.

2. A regression test that fails without the fix — and we verify it goes red when the change is reverted. This is necessary but, on its own, not sufficient: a passing unit test is not evidence of no regression on the heuristic extraction paths.

3. A full-corpus regression run — by you — on an open-source PDF set, with a reference comparison. This is the big one. Our project corpus is private, so please assemble your own set from the public corpora we already validate against — Mozilla pdf.js test/pdfs, Apache PDFBox, the veraPDF corpus, DARPA SafeDocs (a few hundred PDFs across scanned/OCR, CJK/RTL, tagged, forms, damaged). Then:

   • Build corpus_sig, run it on that set for both main and your branch, and diff the two (exact commands are in CONTRIBUTING.md → Testing and regression requirements).
   • Judge the diff with a structural/reference metric — word-Jaccard + spacing outliers, not char-Levenshtein — and report it in the PR: "corpus I tested (count + kinds + source)" and the delta.
   • Crucially, this must verify two things, not one: (a) that your change does what it claims on the PDFs it targets, and (b) that it doesn't perturb the related logic it shares code with. Nearly every one of these fixes touches a shared path — a change to off-MediaBox culling, /PlacedPDF suppression, or the Identity/CID cascade can silently introduce new word-fusions, over-splits, dropped spans, or reversed scripts elsewhere. The corpus diff is what catches that collateral behavior; the fixture never will.

A concrete example: this PR (#909) is logically sound but ships no test and no corpus diff, so we can't lock in the behavior or rule out side-effects — a broken-/Count fixture plus a corpus_sig diff showing zero unintended deltas would make it merge-ready immediately.

And a pacing request: please keep only a couple of PRs in flight at once (or open them as drafts until they're self-verified per the above), rather than the whole batch. That keeps the CI queue clear and gets each of yours a fast, clean run instead of getting cancelled to make room.

To be clear, this isn't a reflection on the work — it's the opposite. These are exactly the practices in #901, and adopting them now means your changes land with far fewer round-trips and much faster. Genuinely grateful for the contributions, and looking forward to merging more.

[ajbufort]

Thanks Yury — test added, and it lands the "0 → N" it was missing.

It's a self-built broken-/Count fixture: a /Pages node with /Count 0 but three real /Kids, so get_page_count_standard() returns 0 (the test asserts that directly) and the enumerator rescue recovers the real count of 3. I verified it goes red (0 != 3) when the document.rs rescue hunk is reverted, and green with it — so it pins the behaviour, not just the happy path. (An ObjStm-packed /Pages reaches page_count the same way, via primary == Ok(0); the plain broken-/Count shape is the minimal self-contained reproduction, no third-party PDF needed.)

Both minor notes are in too:

• Comment tightened. The outer comment no longer implies get_page reaches pages via all_page_refs. It now states the actual premise — the /Count readers and all_page_refs/collect_page_refs both miss these trees, while get_page's own per-page traversal / collect_all_pages reaches them — so it matches the inner comment.
• Perf acknowledged. Added the one-liner that for an ObjStm-packed tree each get_page can fall back to a full object scan, so counting is O(n·objects) — bounded by the sanity cap, and only ever on an already-broken document.

And thank you for running the 419-corpus regression. Agreed that 0 diffs is the correct, desired result here: the primary == Ok(0) guard makes every well-formed document byte-identical, so the gain can only appear on the broken-/Count / ObjStm-packed docs the public suite doesn't contain — which is exactly what the fixture now covers.

[ajbufort]

And separately — thank you for laying out the contribution policy (#901) so clearly, and for the generous framing. It's the right bar, and it makes our side better too; we're glad to adopt it across the board.

Going forward on every behavioral PR we'll:

1. Issue first. Open an accepted issue before the code — the problem, a self-built minimal PDF (or a description of the structure; never third-party/copyrighted PDFs as fixtures), the ISO 32000 clause, and the expected before→after — and reference it with Closes #NNN.
2. A test that fails without the fix. Ship a regression test we've verified goes red on revert (as on fix(page-count): recover pages when /Count-based counting returns 0 #909 above), understanding it's necessary-but-not-sufficient on the heuristic paths.
3. A full corpus_sig diff, run by us. On the public sets you named (pdf.js test/pdfs, PDFBox, veraPDF, DARPA SafeDocs), main vs branch, judged with word-Jaccard + spacing outliers (not char-Levenshtein) — and reported in the PR with the corpus (count + kinds + source) and the delta. Crucially both directions: that the change does what it claims and that it doesn't perturb the shared paths it touches (off-MediaBox culling, /PlacedPDF suppression, the Identity/CID cascade, etc.).

On pacing: understood re: the ~215-check matrix and one-PR-at-a-time runners. We'll keep only a couple in flight and move the rest to draft until each is self-verified per the above, so your queue isn't saturated and the active ones get clean, fast runs.

Genuinely appreciate the detailed reviews and the corpus runs you've been doing on our behalf — that's exactly the load we should be carrying, and we'll take it from here.

[yfedoseev]

Re-reviewed — this is cleared. Thanks for the fixture and the comment/perf cleanup.

Confirmed the problem is real (before approving): disabled just the rescue (if false && matches!(...)) and the new test fails —

assertion `left == right` failed: rescue must enumerate the real pages

i.e. without the fix, page_count() on the broken-/Count fixture returns 0 instead of 3. Restoring the rescue, the test passes. So the test is genuinely revert-checked and the defect reproduces.

No corpus regression: the rescue block is gated on primary == Ok(0), so well-formed documents are byte-identical — confirmed 0 diffs across the 419-corpus (the fix logic is unchanged from that run; this update only added the test + tightened comments).

Approving and adding to the merge queue (ahead of the governance batch).

[yfedoseev]

Approved — bug reproduced (revert-checked test), no corpus regression, review clean. Queued ahead of governance.