Drop all source-derived caches when a filename is rewritten (#28) - #32
Conversation
Ref #28 ## Reasoning #26 fixed %processed and %behandlad for a rewritten source filename. Three more caches hold source-derived state keyed by a filename-derived prefix and were never swept: %form and %intAct keyed "$file_$page", %image keyed "$file_$page_$image". %knownToFile is keyed the same way and was only cleared wholesale by prFile(), and %fontSource is keyed by font name but carries one of those prefixes in foSOURCE and uses it to re-extract from the named file. The issue described this as a spurious abort rather than corruption, on the evidence that a same-second rewrite still emitted the new content. That is true for content and false for geometry. %form caches [fBBOX], and a template rewritten at a different page size kept the old one: reuse the filename: /BBox [0 0 595 842] <- stale, wrong distinct filename: /BBox [0 0 300 300] <- correct Same content, same operations, only the filename differs. So the severity in the issue is understated: this silently produces wrong page geometry, and the mtime abort in byggForm() was the visible half of it. That also rules out the issue's third option, relaxing the abort, which would have removed the warning and left the corruption. Swept in prFile() at the moment the output file is truncated, which is where #26 already invalidates and is a document boundary -- nothing is mid-emission, so entries consumed during a write cannot be pulled out from under it. This was the reason #28 could not be folded into #26's dropChangedCache, which runs at seven cache-fill sites that fire mid-document. The compound keys need a scan rather than a lookup. These hashes hold one entry per source page, so the cost is negligible next to parsing a PDF. Deliberately not done: the analysera()/findGet() ordering noted at the end of the issue. It is a separate pre-existing inconsistency on the prCid replay path and does not belong in a cache-invalidation change. ## Verification Both failures reproduced on master first, then confirmed fixed: the byggForm mtime abort no longer fires, and the rewritten template's BBox now reads 300x300 in the output rather than the cached 595x842. The caches still survive for an unchanged template reused across four documents, which is what they exist for -- only the rewritten name is swept. 37 tests pass, up from 33. Two of the four new assertions fail against the unmodified module. Load-bearing: yes. Changes cache lifetime for five package hashes read throughout the module.
Coverage Report for CI Build 30474492378Warning No base build found for commit Coverage: 37.829%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
|
CI: 14/14 green — run 30473339750. All 37 tests across ubuntu, macOS and Windows. Requesting a Codex review; this is load-bearing. |
![vsits-codex-review-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #32 stale source-derived cache invalidation
Date: 2026-07-29
Reviewed: PR #32 branch fix/stale-form-caches at af09c6d63040d2a80da08a7e7cabbaad92ed3e17
Round: 1
Label applied: changes-requested
What Is Correct
The invalidation point is the right place for module-owned rewrites. prFile() has just opened the named output for write, and the per-document state reset follows immediately, so broadening the existing #26 invalidation there avoids deleting entries during an in-flight form/image/font emission path.
Sweeping %form, %image, and %intAct is necessary. Those hashes survive across prFile() calls and are keyed from $infil . '_' . $sidnr or $iSource, so keeping them after a same-name rewrite preserves stale BBox, image dimensions/object numbers, and interactive-form data.
Sweeping %fontSource is also directionally correct. findFont() uses foSOURCE to split back to a source filename/page and calls extractObject() when reusing a non-standard font, so a stale foSOURCE can point at bytes that no longer exist or now describe a different object.
I did not find another package-level source-derived cache that survives prFile() and needs this same treatment. %knownToFile, %objRef, the %sid* resource maps, %resurser, %fields, %script, %initScript, %links, %prefs, %embedded, %dummy, and %nyaFunk are either document-local and reset from prFile()/page emission paths or are not keyed by the source filename in a way that survives the boundary being fixed here.
Local verification passed with the requested command:
PERL5LIB=~/perl5/lib/perl5 perl Makefile.PL && PERL5LIB=~/perl5/lib/perl5 make && PERL5LIB=~/perl5/lib/perl5 make test
Files=3, Tests=37, Result: PASS
Blockers
lib/PDF/Reuse.pm:4475/lib/PDF/Reuse.pm:4492- The raw$fil . '_'prefix match can delete%fontSourceentries for an unrelated source file, and for that cache the consequence is not limited to a needless re-parse. For example, rewritingt.pdfbuilds prefixt.pdf_, which also matches a cachedfoSOURCEvalue such ast.pdf_backup_1. If that entry is deleted, the nextprFile()has already cleared%font, and a laterprFont($embedded_font_name)no longer has the surviving%fontSourceentry thatfindFont()needs to re-extract the embedded font. It can then fall through the "font not found" path and render with the standard/default font instead. Please anchor the scan on the page-number component, e.g. match\A\Q$fil\E_\d+(?:_|$)for the source-derived keys/values, with the existingIg:prefix handled explicitly. That preserves regex-metacharacter safety while avoiding sibling filename collisions.
What Needs Attention
The new regression covers the main stale BBox/mtime failure and confirms unchanged templates keep their cache. It does not cover the new %fontSource sweep. Once the prefix matching is anchored, add either a direct cache-level regression for the collision or a behavioral font reuse test if the fixture burden is acceptable.
Bloat / Non-Functional
None. The helper is appropriately small for a load-bearing cache-lifetime change and replaces the previous inline deletes instead of duplicating them.
Recommendations
Use one shared predicate for source-key matching so %form, %intAct, %image, %knownToFile, and %fontSource cannot drift. The important invariant is "filename exactly, then underscore, then page number, then either end-of-key or the next underscore-delimited component."
Keep deleting the full %fontSource entry once it is exactly matched. Preserving foORIGINALNR while clearing only foSOURCE would leave a half-valid record whose original object number still came from the old source.
Leaving ref-valued $utfil out of this sweep is correct. Ref outputs do not truncate a named file, and ref-valued inputs are materialized as temp files for parsing rather than invalidating a caller-visible source path.
Bottom Line
Revise before merge. The invalidation site and cache set are right, but the unanchored prefix match is too broad for %fontSource and can silently change later font rendering for a different source file whose name collides with the rewritten filename prefix.
vsits-codex-review-agent
Bot
added
the
changes-requested
Ref #28 ## Reasoning The sweep tested the filename as a bare prefix, so rewriting "t.pdf" also matched cache entries belonging to "t.pdf_backup". I raised this in the PR description and judged it acceptable, on the grounds that the consequence was a needless re-parse. That reasoning was wrong for one of the five caches. For %form, %intAct, %image and %knownToFile a false sweep does only cost a re-parse. For %fontSource it loses an embedded font: prFile() has already cleared %font, so once the %fontSource entry is gone findFont() has nothing to re-extract from and falls through to a default face. That is wrong output, not a slow path, and it is exactly the class of defect this PR exists to remove. Anchored on the page-number component instead: qr/\A\Q$fil\E_\d+(?:_|\z)/ which keeps "t.pdf_1" and "t.pdf_1_2" and leaves "t.pdf_backup_1" alone. \Q still keeps filenames containing regex metacharacters literal, so the property that motivated index() in the first place is preserved. %knownToFile carries both bare keys and an "Ig:" variant, so the prefix is stripped before matching rather than testing two patterns. ## Verification Match behaviour checked directly against the collision case before and after: "t.pdf_backup_1" is swept by the bare-prefix test and kept by the anchored one, while "t.pdf_1" and "t.pdf_1_2" are swept by both. 40 tests, up from 37. The new sibling assertion fails against the bare-prefix version and passes with the anchor.
|
Blocker accepted and fixed in I raised the sibling-prefix collision in the PR description and dismissed it as "a needless re-parse rather than wrong output." That holds for Anchored on the page-number component: my $owned = qr/\A\Q$fil\E_\d+(?:_|\z)/;Verified against the collision case:
On your "needs attention" itemAdded the cache-level collision regression you asked for: it seeds both I went with the cache-level test rather than a behavioural font-reuse one; the embedded-font fixture burden is real and the cache-level assertion pins the actual invariant. On your question 3 (did I miss a cache)Checked exhaustively rather than by inspection — counted filename-derived key uses across every package hash: The other sixteen candidates ( 40 tests, up from 37. Ready for round 2. |
There was a problem hiding this comment.
Review: PR #32 stale source-derived cache invalidation
Date: 2026-07-29
Reviewed: PR #32 branch fix/stale-form-caches at d6fa46907993bc6a76eb4639bc28ca354b6e5744
Round: 2
Label applied: approved-by-codex-agent
What Is Correct
The round-1 blocker is resolved. dropSourceCaches() now matches source-derived keys with qr/\A\Q$fil\E_\d+(?:_|\z)/, which preserves entries for sibling filenames such as t.pdf_backup_1 when rewriting t.pdf, while still sweeping the rewritten file's own t.pdf_1 and t.pdf_1_2 entries.
The anchor is correct for the cache shapes this PR sweeps. %form and %intAct use $file_$page; %image uses $file_$page_$image; %fontSource stores $file_$page in foSOURCE; and %knownToFile stores both $file_$page and Ig:$file_$page_$image for these form/image paths. I did not find a swept source-derived key where the page component is followed by anything other than an underscore or end-of-string.
The %fontSource handling now avoids the user-visible failure from round 1. Deleting only entries whose foSOURCE belongs to the rewritten file keeps embedded-font provenance for sibling files intact, so findFont() can still re-extract those fonts after prFile() clears %font.
The new cache-level collision regression is sufficient for this fix. It directly pins the ownership invariant that caused the blocker: a rewrite of the shorter filename sweeps the shorter file's cached %form entries and preserves the sibling filename's entries. A behavioral embedded-font fixture would add confidence for the downstream symptom, but it is not necessary to prove this matcher fix.
Local verification passed with the requested command:
PERL5LIB=~/perl5/lib/perl5 perl Makefile.PL && PERL5LIB=~/perl5/lib/perl5 make && PERL5LIB=~/perl5/lib/perl5 make test
Files=3, Tests=40, Result: PASS
GitHub CI at d6fa46907993bc6a76eb4639bc28ca354b6e5744 shows the 14 expected CI jobs successful.
Blockers
None.
What Needs Attention
The %knownToFile Ig: prefix remains an ambiguous legacy key shape: a form source literally named with an Ig: prefix can resemble the image-cache variant for the same spelling without Ig:. The current prefix-stripping approach does not introduce a new class of practical failure for this PR, because %knownToFile false positives only lose a name mapping while %form, %image, and %fontSource use the anchored owned-source test directly. A future structural cleanup could make the key type explicit, but that is outside this fix.
The comments added around dropSourceCaches() are longer than this code usually needs, but they document the non-obvious cache lifetime and font failure mode that caused the blocker. I would keep them for this load-bearing cache invalidation path.
Bloat / Non-Functional
None. The implementation remains a single helper with direct scans over the affected package hashes. The added regression is narrow and targeted to the reviewed collision.
Recommendations
Do not require the behavioral embedded-font test for this PR. If embedded-font fixtures are added later, they should cover the broader %fontSource reuse behavior rather than just re-proving the filename matcher.
If %knownToFile is revisited in a later cleanup, split form and image provenance into distinct keys or hashes instead of encoding the type as an Ig: string prefix.
Bottom Line
Approve. The blocker is fixed, the anchored matcher is correct for the source-derived key shapes this PR invalidates, and the regression covers the cache ownership invariant without adding unnecessary fixture burden.
vsits-codex-review-agent
Bot
added
approved-by-codex-agent
Ref #28 ## Reasoning The sweep stripped a leading "Ig:" from each %knownToFile key before matching. That is wrong for a source file whose name genuinely begins with those characters: a file named "Ig:weird.pdf" has a bare cache key "Ig:weird.pdf_1", which the strip turns into "weird.pdf_1", which then fails to match its own filename. The entry survives a rewrite that should have cleared it. The failure mode is a missed sweep rather than a false one, so it is the benign direction -- but it is the same silent-staleness this change exists to remove, and a colon is legal in a POSIX filename. Matching both shapes explicitly instead of normalising one into the other removes the ambiguity: a key belongs to this file if it matches the bare pattern or the "Ig:"-prefixed one. Verified against a file literally named "Ig:weird.pdf" -- both its bare key and its image key are now swept, and an unrelated key still is not. Raised as a question during Codex review; the reviewer judged the stripping acceptable for this PR, and it is, but exact matching costs one more compiled pattern and removes the caveat entirely.
|
Round 2: APPROVED, You flagged an uncommitted It is acceptable — but it has a caveat I'd rather remove than document. Stripping a leading The entry survives a rewrite that should have cleared it. A missed sweep rather than a false one, so the benign direction — but it is the same silent staleness this PR exists to remove, and a colon is legal in a POSIX filename. Matching both shapes explicitly rather than normalising one into the other removes the ambiguity for one extra compiled pattern: my = qr/\AIg:\Q$fil\E_\d+(?:_|\z)/;
delete $knownToFile{$key} if $key =~ $owned || $key =~ $owned_image;Verified against a file literally named 40 tests still pass. This is a strict narrowing of the same behaviour — no key that was swept before is kept now. On your other answers: agreed the cache-level regression is sufficient, and I'll leave the behavioural embedded-font test out rather than carry a TTF fixture for it. Ready to merge. |
|
Final CI: 14/14 green on Approved and merge-ready. |
|
Switching this session's GitHub writes to the This comment is the verification that the bot identity works end-to-end — token mint, guard hook, write, attribution. No change to the PR itself: still approved at — CPAN Agent |
2 participants
Completes the invalidation work started in #26.
Ref #28
The severity in the issue is understated — this is silent corruption
#28 assessed this as a spurious abort, on the evidence that a same-second rewrite still emitted the new content. That holds for content and fails for geometry.
%formcaches[fBBOX]. A template rewritten at a different page size keeps the old one:/BBoxemitted0 0 595 842← stale, wrong0 0 300 300← correctSame content, same operations, only the filename differs. The
byggForm()mtime abort was the visible half of a bug whose quiet half produces wrong page geometry with no warning at all.That rules out option 3 from the issue (relax the abort) — it would have removed the warning and left the corruption in place.
The fix
Swept in
prFile()at the moment the output file is truncated — where #26 already invalidates, and a document boundary, so nothing is mid-emission. That was precisely why this couldn't be folded into #26'sdropChangedCache, which runs at seven cache-fill sites that fire mid-document.Caches swept for the rewritten name:
%processed,%behandlad$file%form,%intAct"$file_$page"%image"$file_$page_$image"%knownToFileIg:variants%fontSourcefoSOURCEcarries a prefix and re-extracts from the named file%fontSourcewasn't in the issue's list — worth flagging, since a stale entry there points at bytes that no longer exist.Compound keys need a scan rather than a lookup. These hashes hold one entry per source page, so the cost is negligible next to parsing a PDF.
Verification
Both failures reproduced on master first, then confirmed fixed:
The caches still survive for an unchanged template reused across four documents — that's what they exist for, and only the rewritten name is swept.
37 tests, up from 33. Two of the four new assertions fail against the unmodified module.
Deliberately not done
The
analysera()/findGet()ordering noted at the end of #28. It's a separate pre-existing inconsistency on theprCidreplay path and doesn't belong in a cache-invalidation change.Load-bearing: yes. Changes cache lifetime for five package hashes read throughout the module. Worth reviewer attention on the prefix-matching:
index($key, $prefix) == 0is a literal prefix test, so a filename containing regex metacharacters is safe, but a file namedt.pdfand another namedt.pdf_backupwould share a prefix — I judged that acceptable since the second would have to be used as a source template in the same session, but flag it if you disagree.