From aafc1d1c286c7ba88f0b3132bf2b39c3dd8352e6 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 04:48:17 -0700 Subject: [PATCH] P5a review: outlines from named destinations; pool race; fast over-limit; revalidation under the limiter; Accept pdf - pdftext/outline: resolve named destinations through the catalog /Dests and the /Names /Dests tree, resolve indirect /Dest, /A and /Title, and treat reader.Null (what Dict.Get returns for a missing key) as absent. Before this every LaTeX and Acrobat PDF had an empty outline. pdftest gains NamedDests; TestOutlineNamedDestinations. - api: pool() always goes through its Once (data race on concurrent first use); TestDefaultPoolConcurrentFirstUse. Revalidation of a stale cache entry counts against the caller/host limit. - pdftext/runner: output over MaxText kills the child at once instead of waiting for the deadline; TestRunnerTooMuchTextFailsFast. - fetch: Accept includes application/pdf;q=0.8. Co-Authored-By: Claude Opus 5.5 --- docs/implementer-log.md | 26 +++++ internal/api/api.go | 2 +- internal/api/pdf.go | 15 ++- internal/api/pdf_review_test.go | 30 +++++ internal/fetch/pdf.go | 2 +- internal/pdftest/pdftest.go | 21 ++++ internal/pdftext/outline.go | 187 ++++++++++++++++++++++---------- internal/pdftext/pdftext.go | 2 +- internal/pdftext/review_test.go | 60 ++++++++++ internal/pdftext/runner.go | 7 +- 10 files changed, 287 insertions(+), 65 deletions(-) create mode 100644 internal/api/pdf_review_test.go create mode 100644 internal/pdftext/review_test.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 85bbbb7..45ef4f6 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -12,3 +12,29 @@ One row per task session that ends in a commit. Newest last. | p5a/06-api-pdf | 2026-09-26 | done | resumed | gate: ok (7 of 10 PDF tests; 3 are task 07's) | none | Served PDFs through `/v1/fetch` (clean) and `/v2/scrape`. Added `RetryAfter int` to `fetch.Error` and a `Retry-After` header line guarded by `> 0`. Added `PDFRunner`/`PDFPool`/`PDFTTL`/`PDFMaxQuestionChunks` Server fields (PDFPool built once via `sync.Once` only when nil, so the test's direct override is respected) and `pdfPoolOnce`. Routed `clean.Kind` in `doFetch`/`cleanFetched` (html/json/text switched, "other"→415, "pdf"→extractPDF); `doFetch`'s switch routes "text" explicitly so a PDF never runs `clean.Text` first. New `internal/api/pdf.go`: `DefaultPDFTTL`/`DefaultPDFMaxQuestionChunks` (task 07), `runner()`/`pool()` defaults, `extractPDF` (pool key `callerClass|SHA256`, two-value assert, `fmt.Errorf("pdf: bad result %T")`), `pdfError` (422 for the five pdftext errors + panic default, 503+RetryAfter 10 for ErrBusy, 429 for ErrCallerBusy), `renderPDF` (`pdfdoc.Render`, whole text or map). `doFetch` returns the 4th `doc *pdftext.Doc`; digest uses `pdfdoc.Full(doc)` (computed once) as content, clean uses `renderPDF.Content`, envelope `pdf` object has pages/title/outline/extracted/first/last/map/grep_hits/note, raw mode→400. Scrape markdown=`renderPDF.Content`, html="", `metadata.numPages`=doc.Pages. 7 of 10 given tests pass under -race -count=3; the 3 failures (`TestPDFPagesAndGrep`, `TestPDFCache`, `TestPDFQuestionTooLong`) are task 07 tests that reference fields not in this task, exactly as the task doc (lines 145, 157-158) predicts; nothing else fails. Given files byte-identical. | | p5a/08-digest-cli-docs | 2026-09-26 | done | 1 | gate: ok | none | Added the page-cite sentence to the digest `systemPrompt` ("When the document has `--- page N ---` markers, cite the page of each fact as (p. N).") and set `PromptVersion = "3"`. Added the `sift_pdf_extract_total{outcome}`, `sift_pdf_extract_seconds`, `sift_pdf_pages_total`, `sift_pdf_pool_wait_seconds`, `sift_pdf_cache_hits_total` series to `internal/metrics` (`pdf.go` package) and wired `ObservePDF`/`ObservePDFWait`/`PDFCacheHit` into `internal/api/pdf.go` (`ObservePDF` once per call after `extractPDF`, outcome classified by `pdfOutcome` so raw text never becomes a label; `ObservePDFWait` only when the caller actually waited on `pool().Do`; `PDFCacheHit` on fresh hits and 304s). Refactored `cmd/sift/fetchcmd.go` into a `fetchOpts` struct with `parseFetchOpts` (returns `fetchOpts`, not `string, error`) and a `fetchBody(o fetchOpts) map[string]any` that does the digest/clean default and adds `pages`/`grep` only when set; `--pages`/`--grep` added to `fetchValueFlags`. Updated `skill/SKILL.md` (PDFs section; removed PDF mention from the 415 row). Unit: `nix/module.nix` `MemoryMax = "1G"` (one line). README: added a "PDF documents" section covering detection, limits (100 MB/120 s download, 60 s extraction, 2,000 pages), the child process, the 2-slot/4-wait/20 s pool, the 24 h conditional-GET cache, the answers, the task-06 errors table, and the metrics; noted the `go-pdfkit/extract` v0.3.0 engine chosen by the `docs/P5a.md` §3 bake-off. All tests pass under -race, make gate ok, three given files byte-identical, nix diff is one line. | | p5a/07-api-cache-pages | 2026-09-26 | done | 1 | gate: ok | none | Implemented the PDF document cache with revalidation in `doFetch`: `pdfFromCache` (miss / fresh within `PDFTTL` / stale → `FetchIf` with 304→`TouchPDF`+answer-fresh / 200→return response with ok=false and no second GET) and `storePDF` after `extractPDF` (`ContentHash=res.SHA256`, ETag/Last-Modified from headers, FetchedAt=now, Meta=JSON of `doc.Meta`, Pages=`doc.Text`). `pages`/`grep` passed into `pdfdoc.Options` in `renderPDF`; `ErrBadPages`/`ErrBadGrep`→400 with the error's own text, non-PDF→400 "pages and grep apply to PDFs only" before the doc block. Question limit in digest mode: `n=len(digest.Split(pdfdoc.Full(doc), ChunkTokens))`, `n>PDFMaxQuestionChunks`→422 "document too long for question (n chunks); use grep or pages" before any digest call. Envelope `cached` set true for cache answers via the synthetic cache res's empty body (`len(res.Body)==0`); the one `doFetch` change beyond the rule-1 call was reusing that 200 response instead of a second `Fetch`. Added `fmt` import to api.go. Given file byte-identical; make gate ok. | + +## P5a review (owner, 2026-09-26) + +Checked: 8 task commits (`13365a2` … `7e78de6`), each with the `Co-Authored-By: ornith-1.5-35b-a3b` +trailer; every given file byte-identical to its plan copy; `internal/guard`, `internal/auth`, +`go.mod`, `go.sum`, `vendor/` untouched; `nix/module.nix` changed only in `MemoryMax`; `make gate` +ok; `go test -race -count=3 ./...` ok. + +End-to-end with the real binary, the real guard, a real cache and dixie's 9B for digests, on +public PDFs: Attention (15 pp, whole text, 1.9 s), GPT-3 (75 pp, map, 2.6 s), NIST SP 800-63B, +the RP2040 datasheet (642 pp, 4.5 s), Karpathy's thesis (14.7 MB, 16 s including download). +`pages` and `grep` from the cache in about 1.5 ms; a `question` digest with correct page +citations (p. 1, 7, 8) in 11 s; `/v2/scrape` shape; `sift_pdf_*` metrics; no title or text in +the logs. + +| # | Finding | Severity | Fault | Fix | +|---|---|---|---|---| +| 1 | **No outline for any LaTeX (hyperref) or Acrobat PDF** — every arXiv paper and the NIST PDF showed 0 items (qpdf: 22–120). Named destinations were skipped (task 02 said to skip them), `/Dest`/`/A`/titles were not resolved when indirect, and reader's `Dict.Get` returns `reader.Null{}` for a missing key, so the `/A` branch never ran. | high (feature absent on most papers) | task + model | owner: name-tree and `/Dests` lookup, `resolve` treats Null as absent, indirect titles; `pdftest` `NamedDests`; `TestOutlineNamedDestinations` (fails on the old code) | +| 2 | `pool()` read `PDFPool` outside its `sync.Once` — a data race on concurrent first use with no pool configured (the production default). | medium | model | owner: always through the Once; `TestDefaultPoolConcurrentFirstUse` (race on the old code) | +| 3 | Output over `MaxText` returned only at the deadline (the runner waited for stderr EOF while the child blocked on a full stdout pipe). | low | model | owner: check size first; `TestRunnerTooMuchTextFailsFast` (30 s on the old code) | +| 4 | Stale-cache revalidation ran outside `acquire`, so it was not counted against the per-caller/host limit. | low | task (said "before acquire") | owner | +| 5 | Fetch `Accept` lacked `application/pdf` (P5a §4). | low | task (not in task 01's rules) | owner | + +Process: 1 owner test deadlock (task 01, 55 min), 1 oversized task (06, looped 3 h, split), +2 hangs from OpenCode 1.15 waiting on a piped stdin (driver fixed), 1 refusal-ending (task 01), +1 malformed tool call (task 04). Every task that ran to the gate passed it first time. diff --git a/internal/api/api.go b/internal/api/api.go index 800f775..715a057 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -314,7 +314,7 @@ type v1Req struct { JQ string `json:"jq"` Recover bool `json:"recover"` - // Pages narrows a PDF digest to "2-3"; Grep matches lines in it. Optional; task-07. + // Pages ("41-60") and Grep (an RE2 pattern) narrow a PDF answer in clean mode; see pdfdoc. Pages string `json:"pages"` Grep string `json:"grep"` } diff --git a/internal/api/pdf.go b/internal/api/pdf.go index 2fd0727..9804d08 100644 --- a/internal/api/pdf.go +++ b/internal/api/pdf.go @@ -35,11 +35,12 @@ func (s *Server) runner() *pdftext.Runner { // pool returns the extraction pool, built once and only when none is configured. func (s *Server) pool() *pdfpool.Pool { - if s.PDFPool == nil { - s.pdfPoolOnce.Do(func() { + // Always through the Once: reading PDFPool outside it would race with the first writer. + s.pdfPoolOnce.Do(func() { + if s.PDFPool == nil { s.PDFPool = pdfpool.New(2, 4, 20*time.Second) - }) - } + } + }) return s.PDFPool } @@ -158,7 +159,13 @@ func (s *Server) pdfFromCache(ctx context.Context, c *auth.Caller, rawURL string res, doc := s.pdfFromEntry(rawURL, e) return res, doc, true, nil } + // A revalidation is an upstream request like any other: it counts against the caller's and + // the host's limits. + if err := s.acquire(c.Name, rawURL); err != nil { + return nil, nil, false, err + } res, err := s.Fetch.FetchIf(ctx, rawURL, e.ETag, e.LastModified) + s.release(c.Name, rawURL) if err != nil { return nil, nil, false, err } diff --git a/internal/api/pdf_review_test.go b/internal/api/pdf_review_test.go new file mode 100644 index 0000000..a8dc92d --- /dev/null +++ b/internal/api/pdf_review_test.go @@ -0,0 +1,30 @@ +package api + +// Owner review (P5a): the default pool is built once, safely, when concurrent first requests +// arrive on a server with no pool configured (run under -race). + +import ( + "sync" + "testing" +) + +func TestDefaultPoolConcurrentFirstUse(t *testing.T) { + s, _ := setupAPI(t, nil) + var wg sync.WaitGroup + got := make(chan any, 16) + for i := 0; i < 16; i++ { + wg.Add(1) + go func() { defer wg.Done(); got <- s.pool() }() + } + wg.Wait() + close(got) + var first any + for p := range got { + if first == nil { + first = p + } + if p != first || p == nil { + t.Fatal("pool() returned different or nil pools") + } + } +} diff --git a/internal/fetch/pdf.go b/internal/fetch/pdf.go index 1f5bb76..636adc8 100644 --- a/internal/fetch/pdf.go +++ b/internal/fetch/pdf.go @@ -101,7 +101,7 @@ func (f *Fetcher) doFetch(ctx context.Context, rawURL, etag, lastModified string return nil, errf(400, "", "invalid url") } req.Header.Set("User-Agent", f.ua()) - req.Header.Set("Accept", "text/html,application/xhtml+xml,application/json;q=0.9,text/plain;q=0.8,*/*;q=0.1") + req.Header.Set("Accept", "text/html,application/xhtml+xml,application/json;q=0.9,text/plain;q=0.8,application/pdf;q=0.8,*/*;q=0.1") if etag != "" { req.Header.Set("If-None-Match", etag) } diff --git a/internal/pdftest/pdftest.go b/internal/pdftest/pdftest.go index 62508de..20de7ff 100644 --- a/internal/pdftest/pdftest.go +++ b/internal/pdftest/pdftest.go @@ -25,6 +25,10 @@ type Doc struct { Pages []string Outline []Outline Scanned []int + // NamedDests writes the outline the way LaTeX's hyperref and Acrobat do: each item is a + // /GoTo action to a named destination (resolved through the catalog's /Names /Dests tree) + // and its /Title is an indirect object. Without it items carry a direct /Dest array. + NamedDests bool } // Build returns the PDF bytes for d. It panics on a Doc it cannot express (an outline entry @@ -51,9 +55,15 @@ func Build(d Doc) []byte { scanned[p] = true } + // With NamedDests the outline items are followed by one title object per item and then + // the name tree, so the numbers below stay fixed by position. + nameTree := outlineRoot + 1 + 2*len(d.Outline) catalog := "<< /Type /Catalog /Pages 2 0 R" if len(d.Outline) > 0 { catalog += fmt.Sprintf(" /Outlines %d 0 R /PageMode /UseOutlines", outlineRoot) + if d.NamedDests { + catalog += fmt.Sprintf(" /Names << /Dests %d 0 R >>", nameTree) + } } obj(catalog + " >>") var kids []string @@ -93,6 +103,9 @@ func Build(d Doc) []byte { } n := first + i item := fmt.Sprintf("<< /Title (%s) /Parent %d 0 R /Dest [%d 0 R /XYZ null null null]", esc(o.Title), outlineRoot, pageObj(o.Page-1)) + if d.NamedDests { + item = fmt.Sprintf("<< /Title %d 0 R /Parent %d 0 R /A << /S /GoTo /D (dest.%d) >>", last+1+i, outlineRoot, i) + } if i > 0 { item += fmt.Sprintf(" /Prev %d 0 R", n-1) } @@ -101,6 +114,14 @@ func Build(d Doc) []byte { } obj(item + " >>") } + if d.NamedDests { + var names []string + for i, o := range d.Outline { + obj(fmt.Sprintf("(%s)", esc(o.Title))) + names = append(names, fmt.Sprintf("(dest.%d) [%d 0 R /XYZ null null null]", i, pageObj(o.Page-1))) + } + obj(fmt.Sprintf("<< /Names [%s] >>", strings.Join(names, " "))) + } } xref := b.Len() diff --git a/internal/pdftext/outline.go b/internal/pdftext/outline.go index fd08bcc..9073be9 100644 --- a/internal/pdftext/outline.go +++ b/internal/pdftext/outline.go @@ -24,27 +24,33 @@ func walkOutline(doc *reader.Document) []OutlineItem { if !ok { return out } - outlines, err := doc.Resolve(catDict.Get("Outlines")) - if err != nil || outlines == nil { - return out - } - outlinesDict, ok := reader.ToDict(outlines) + outlines, ok := reader.ToDict(resolve(doc, catDict.Get("Outlines"))) if !ok { return out } - first, ok := toRef(outlinesDict.Get("First")) + first, ok := toRef(outlines.Get("First")) if !ok { return out } + o := &outliner{doc: doc, cat: catDict} seen := map[reader.Ref]bool{} - walkChildren(doc, first, 1, seen, &out) + o.walkChildren(first, 1, seen, &out) return out } +// outliner resolves destinations for one document. The page index and the named +// destinations are built once, on first use. +type outliner struct { + doc *reader.Document + cat reader.Dict + pages map[reader.Ref]int + names map[string]reader.Object // named destinations from /Dests and the /Names /Dests tree +} + // walkChildren walks the /Next chain from ref at the given level, emitting each // resolvable item and recursing into its /First children (level + 1). It stops // after maxOutlineItems items and never visits an object twice. -func walkChildren(doc *reader.Document, ref reader.Ref, level int, seen map[reader.Ref]bool, out *[]OutlineItem) { +func (o *outliner) walkChildren(ref reader.Ref, level int, seen map[reader.Ref]bool, out *[]OutlineItem) { cur := ref for cur != (reader.Ref{}) { if seen[cur] { @@ -52,24 +58,20 @@ func walkChildren(doc *reader.Document, ref reader.Ref, level int, seen map[read } seen[cur] = true - obj, err := doc.Resolve(cur) - if err != nil || obj == nil { - break - } - d, ok := reader.ToDict(obj) + d, ok := reader.ToDict(resolve(o.doc, cur)) if !ok { break } - if page, ok := destPage(doc, d); ok { - *out = append(*out, OutlineItem{Title: textString(d.Get("Title")), Page: page, Level: level}) + if page, ok := o.destPage(d); ok { + *out = append(*out, OutlineItem{Title: textString(resolve(o.doc, d.Get("Title"))), Page: page, Level: level}) if len(*out) >= maxOutlineItems { return } } if child, ok := toRef(d.Get("First")); ok { - walkChildren(doc, child, level+1, seen, out) + o.walkChildren(child, level+1, seen, out) if len(*out) >= maxOutlineItems { return } @@ -83,57 +85,130 @@ func walkChildren(doc *reader.Document, ref reader.Ref, level int, seen map[read } } -// destPage resolves an item's destination to a 1-based page number. It reports -// ok only when the destination is an array whose first element is a page -// reference that maps to a real page; a named or otherwise unresolvable -// destination is not ok, so the caller skips the item. -func destPage(doc *reader.Document, d reader.Dict) (int, bool) { - if ref, ok := destRef(doc, d); ok { - if page, ok := pageOf(doc, ref); ok { - return page, true - } - } - return 0, false -} - -// destRef finds the destination page reference, from /Dest or from a /GoTo -// action's /D. -func destRef(doc *reader.Document, d reader.Dict) (reader.Ref, bool) { - if o := d.Get("Dest"); o != nil { - if arr, ok := reader.ToArray(o); ok && len(arr) > 0 { - if ref, ok := arr[0].(reader.Ref); ok { - return ref, true - } - } - } - if o := d.Get("A"); o != nil { - if a, ok := reader.ToDict(o); ok { +// destPage resolves an item's destination to a 1-based page number: /Dest, or a +// /GoTo action's /D; either may be an explicit array, a dictionary holding /D, +// or a name looked up in the catalog's named destinations (LaTeX's hyperref and +// Acrobat write names, so without them most papers have no outline). +func (o *outliner) destPage(d reader.Dict) (int, bool) { + dest := resolve(o.doc, d.Get("Dest")) + if dest == nil { + if a, ok := reader.ToDict(resolve(o.doc, d.Get("A"))); ok { if s, ok := reader.ToName(a.Get("S")); ok && s == "GoTo" { - if d2 := a.Get("D"); d2 != nil { - if arr, ok := reader.ToArray(d2); ok && len(arr) > 0 { - if ref, ok := arr[0].(reader.Ref); ok { - return ref, true - } - } - } + dest = resolve(o.doc, a.Get("D")) } } } - return reader.Ref{}, false + return o.pageOfDest(dest, 0) } -// pageOf maps a reference to its 1-based page by comparing it with each page's -// reference. -func pageOf(doc *reader.Document, ref reader.Ref) (int, bool) { - for i := 1; i <= doc.PageCount(); i++ { - pr, ok := doc.PageRef(i) - if ok && pr == ref { - return i, true +// pageOfDest maps a destination value to its page. depth bounds the chain +// name → dictionary → array, which a malformed file could make circular. +func (o *outliner) pageOfDest(dest reader.Object, depth int) (int, bool) { + if dest == nil || depth > 4 { + return 0, false + } + switch v := dest.(type) { + case reader.Array: + if len(v) == 0 { + return 0, false } + ref, ok := v[0].(reader.Ref) + if !ok { + return 0, false + } + return o.pageOf(ref) + case reader.Dict: + return o.pageOfDest(resolve(o.doc, v.Get("D")), depth+1) + case reader.Name: + return o.pageOfDest(o.named(string(v)), depth+1) + case reader.String: + return o.pageOfDest(o.named(string(v)), depth+1) } return 0, false } +// pageOf maps a page reference to its 1-based page number. +func (o *outliner) pageOf(ref reader.Ref) (int, bool) { + if o.pages == nil { + o.pages = make(map[reader.Ref]int, o.doc.PageCount()) + for i := 1; i <= o.doc.PageCount(); i++ { + if pr, ok := o.doc.PageRef(i); ok { + o.pages[pr] = i + } + } + } + p, ok := o.pages[ref] + return p, ok +} + +// named returns the destination a name points to, from the PDF 1.1 /Dests +// dictionary or the /Names /Dests name tree. +func (o *outliner) named(name string) reader.Object { + if o.names == nil { + o.names = map[string]reader.Object{} + if dests, ok := reader.ToDict(resolve(o.doc, o.cat.Get("Dests"))); ok { + for k, v := range dests { + o.names[string(k)] = v + } + } + if names, ok := reader.ToDict(resolve(o.doc, o.cat.Get("Names"))); ok { + o.nameTree(resolve(o.doc, names.Get("Dests")), map[reader.Ref]bool{}, 0) + } + } + return resolve(o.doc, o.names[name]) +} + +// maxNames bounds a name tree read; maxTreeDepth bounds its nesting. +const ( + maxNames = 200000 + maxTreeDepth = 32 +) + +// nameTree reads a name-tree node: leaf /Names pairs [key value …] and /Kids. +func (o *outliner) nameTree(node reader.Object, seen map[reader.Ref]bool, depth int) { + d, ok := reader.ToDict(node) + if !ok || depth > maxTreeDepth || len(o.names) >= maxNames { + return + } + if pairs, ok := reader.ToArray(resolve(o.doc, d.Get("Names"))); ok { + for i := 0; i+1 < len(pairs) && len(o.names) < maxNames; i += 2 { + if k, ok := reader.ToString(resolve(o.doc, pairs[i])); ok { + o.names[string(k)] = pairs[i+1] + } + } + } + if kids, ok := reader.ToArray(resolve(o.doc, d.Get("Kids"))); ok { + for _, k := range kids { + if ref, ok := k.(reader.Ref); ok { + if seen[ref] { + continue + } + seen[ref] = true + } + o.nameTree(resolve(o.doc, k), seen, depth+1) + } + } +} + +// resolve follows a reference. A missing, null or unresolvable object is nil: +// reader's Dict.Get answers reader.Null{} for an absent key, not nil. +func resolve(doc *reader.Document, obj reader.Object) reader.Object { + if obj == nil { + return nil + } + if _, isNull := obj.(reader.Null); isNull { + return nil + } + v, err := doc.Resolve(obj) + if err != nil { + return nil + } + if _, isNull := v.(reader.Null); isNull { + return nil + } + return v +} + // toRef reports the reference a dictionary entry names, when it is a direct // reference rather than an already-resolved object. func toRef(o reader.Object) (reader.Ref, bool) { diff --git a/internal/pdftext/pdftext.go b/internal/pdftext/pdftext.go index e109b1a..659f2ee 100644 --- a/internal/pdftext/pdftext.go +++ b/internal/pdftext/pdftext.go @@ -148,7 +148,7 @@ func infoTitle(doc *reader.Document) string { if !ok { return "" } - return textString(d.Get("Title")) + return textString(resolve(doc, d.Get("Title"))) // the title may be an indirect object } // textString decodes a reader.String (raw bytes) as a PDF text string per rule 3. diff --git a/internal/pdftext/review_test.go b/internal/pdftext/review_test.go new file mode 100644 index 0000000..10be140 --- /dev/null +++ b/internal/pdftext/review_test.go @@ -0,0 +1,60 @@ +package pdftext_test + +// Owner review (P5a): output over MaxText fails fast, not at the deadline. The child writes more +// than a pipe buffer (64 KiB), so it blocks on stdout until killed; Run must not wait for it to +// exit on its own before checking the size. + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "testing" + "time" + + "git.wntrmute.dev/kyle/sift/internal/pdftest" + "git.wntrmute.dev/kyle/sift/internal/pdftext" +) + +func TestRunnerTooMuchTextFailsFast(t *testing.T) { + var pages []string + for p := 1; p <= 80; p++ { + pages = append(pages, pdftest.Lorem(p, 40)) // ~2,700 characters a page, ~200 KiB in all + } + r := runner() + r.MaxText = 1000 + r.Deadline = 30 * time.Second + start := time.Now() + _, err := r.Run(context.Background(), pdftest.Build(pdftest.Doc{Pages: pages})) + if !errors.Is(err, pdftext.ErrTooMuchText) { + t.Fatalf("err %v, want ErrTooMuchText", err) + } + if d := time.Since(start); d > 5*time.Second { + t.Errorf("ErrTooMuchText after %v; it waited for the deadline instead of killing the child", d) + } +} + +// Owner review (P5a): LaTeX (hyperref) and Acrobat outlines use named destinations through +// the /Names tree and indirect titles; the first run returned no outline for any arXiv paper +// or the NIST PDF. reader's Dict.Get answers reader.Null{} for a missing key, which hid /A. +func TestOutlineNamedDestinations(t *testing.T) { + b := pdftest.Build(pdftest.Doc{ + Title: "Named", + Pages: []string{"one", "two", "three"}, + Outline: []pdftest.Outline{{Title: "1 Intro", Page: 1, Level: 1}, {Title: "2 Method", Page: 3, Level: 1}}, + NamedDests: true, + }) + var out bytes.Buffer + if err := pdftext.Extract(b, 10, &out); err != nil { + t.Fatal(err) + } + var l struct{ Meta pdftext.Meta } + line, _ := out.ReadBytes('\n') + if err := json.Unmarshal(line, &l); err != nil { + t.Fatal(err) + } + want := []pdftext.OutlineItem{{Title: "1 Intro", Page: 1, Level: 1}, {Title: "2 Method", Page: 3, Level: 1}} + if len(l.Meta.Outline) != 2 || l.Meta.Outline[0] != want[0] || l.Meta.Outline[1] != want[1] { + t.Errorf("outline = %+v, want %+v", l.Meta.Outline, want) + } +} diff --git a/internal/pdftext/runner.go b/internal/pdftext/runner.go index 1d981d5..9f2679e 100644 --- a/internal/pdftext/runner.go +++ b/internal/pdftext/runner.go @@ -106,14 +106,17 @@ func (r *Runner) Run(ctx context.Context, pdf []byte) (*Doc, error) { } readErr := <-done - <-stderrDone - // Output over the limit: kill the child and report. + // Output over the limit: kill the child at once. Checked before waiting for stderr, which + // only closes when the child exits — and a child blocked on a full stdout pipe would not, + // until the deadline. if int64(stdoutBuf.Len()) > maxText { cancel() + <-stderrDone _ = cmd.Wait() return nil, ErrTooMuchText } + <-stderrDone waitErr := cmd.Wait()