fix(files): cap CBR/CBT/CB7 decompressed-bytes per-load + byte-budgeted page cache (bookshelf-t582g.10) #1250
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-t582g.10"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Phase A of bookshelf-t582g.10: prevents OOM from large comic archives.
Two defences added:
Per-load total-byte budget (
maxCBRTotalBytes/maxCBTTotalBytes/maxCB7TotalBytes, default 1 GiB each): CBR/CBT/CB7 loaders now accumulatetotalByteswhile iterating entries and reject any archive whose cumulative decompressed image bytes exceed the budget before they are all buffered. ReturnsErrCBRTotalBytesTooLarge/ErrCBTTotalBytesTooLarge/ErrCB7TotalBytesTooLarge— permanent/non-retryable domain sentinels (no workflow engine import).Byte-budgeted
pageCache[T]:pageCachenow trackstotalBytesalongside the archive count. A newbyteLimitfield (default 512 MiB per cache, tunable via tests) drivesevictLocked()which evicts LRU archives until both the count limit and byte budget are satisfied. Old count-only bound (256 archives) could hold 256 × 1 GiB = 256 GiB; the byte budget closes that gap.Scope: Phase A only. The per-page-decompress redesign (matching CBZ's per-request streaming model) is deferred to Phase D as specified.
Files changed
internal/files/page_cache.go— byte-budget fields +evictLockedhelper +totalCachedBytes()accessorinternal/files/cbr_pages.go— per-load budget + newcbrPageSizeOf, updated cache initinternal/files/cbt_pages.go— per-load budget + newcbtPageSizeOf, updated cache initinternal/files/cb7_pages.go— per-load budget + newcb7PageSizeOf, updated cache init, newErrCB7TotalBytesTooLargeinternal/files/export_test.go— exports for new vars/sentinels +ResetXxxPageCacheWithByteshelpersinternal/files/page_cache_internal_test.go— updated to new 3-paramnewPageCache, byte-budget eviction specsinternal/files/cbr_pages_test.go— total-byte budget + byte-cache eviction specsinternal/files/cbt_pages_test.go— total-byte budget + byte-cache eviction specsinternal/files/cb7_pages_test.go— total-byte budget + byte-cache eviction specsTest plan
make testpasses (all packages green)make coverage— 100% statement coverage oninternal/filesmaintainedCloses bead bookshelf-t582g.10 on merge.
Security Review — PR #1240 (ComicInfo.xml → exact-by-ID provider fetch)
Adversarial focus: ComicInfo.xml is attacker-controlled (inside an untrusted .cbz/.cbr).
SSRF (untrusted
<Web>/<Notes>steering outbound host) — CLEAN. The rawURL is never used as a request target.
extractProviderIDFromComicInfo(
internal/bookdrop/comicinfo_provider_id.go) only regex-captures a numeric issueID (
4000-(\d+),metron\.cloud/issue/(\d+),\[ComicVine:4000-(\d+)\]). Bothproviders build the request from the FIXED provider base URL:
fmt.Sprintf("%s/issue/4000-%d/?%s", baseURL, issueID, ...)(internal/metadata/comicvine/search.go:778),baseURL=defaultBaseURL/test server.fmt.Sprintf("%s/issue/%d/", d.baseURL, issueID)(internal/metadata/metron/provider.go:425).The persisted
WebLinkalso passes throughurlutil.SafeURL(internal/comic/persist.go:163).Injection / request-splitting — CLEAN. The ID is parsed with
strconv.ParseInt(match[1], 10, 64)and rejected unless> 0; it is emitted as%d. No non-numeric content can reach the path/query. Provider tag is a fixedliteral (
"comicvine"/"metron"), never concatenated into a URL.XXE / XML bomb — CLEAN. Parsing uses Go
encoding/xml(decodeComicInfoReader,internal/comic/comicinfo.go:258) with noCharsetReaderand no customEntitymap — external/DTD entities are not resolved by default. Decompressed size is
double-bounded: the zip/rar header uncompressed-size guard rejects
> maxComicInfoBytes(1 MB) before open, and anio.LimitReader(rc, cap)caps thedecode. Archive read is
io.LimitReader(f, 512MB). Zip entry is read, notextracted to disk (no zip-slip).
API keys — CLEAN. New log lines emit only
issue_id/volume_id/err(
internal/metadata/comicvine/provider.go); noapi_keyin the new code.Permanent vs transient — CORRECT. A malformed/missing/unsupported archive →
readComicArchiveerror/ErrNoComicInfois swallowed and falls back to fuzzysearch (
fetchMergedCandidateForProposal). A provider 404 for anattacker-forged ID maps to
metadata.ErrNoMatch(fall through to otherproviders), not a hard error. Exact-by-ID fetch still runs under the existing
rate limiter and only for already-enabled/active providers.
[MINOR] internal/bookdrop/import_metadata_service.go:15 — import out of goimports order
internal/comicis placed afterinternal/db/sqlc(comic < db/sqlc). Ifgolangci-lint/gci is not flagging it, harmless, but reorder for tidiness. No
security impact.
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
[BLOCKER] internal/files/page_cache_internal_test.go — white-box test file added in direct violation of project black-box-only convention
This file declares
package files(white-box/internal test) rather thanpackage files_test(black-box). The project-conventions.md § Testing explicitly forbid this: "Every *_test.go declares package _test, never package . Cover an unexported branch by driving the public caller that reaches it, or delete as dead code."While
page_cache_internal_test.gois pre-existing on the allowlist (grandfathered debt), the PR adds ~60 lines of NEW white-box tests (lines +145–+207) that directly instantiate and test the unexportedpageCache[T]type, calling unexported methods.add(),.get(),.totalCachedBytes(), etc. These new tests should be converted to black-box: the pageCache branches can be driven via public CBRPageCount/CBTPageCount/CB7PageCount with the export_test.go helpers (ResetCBRPageCacheWithBytes, CBRPageCacheTotalBytes, etc.).Why it matters: The convention exists to prevent tests from relying on internal implementation details. White-box tests are brittle and hide dead code. The project has a burn-down epic (bookshelf-nnb9) to eliminate the pre-existing debt; this PR adds new debt in the same file, moving the opposite direction. The allowlist is meant to SHRINK, not grow.
Fix: Convert the new byte-budget eviction tests (lines +145–+207 in page_cache_internal_test.go) to black-box tests under cbr_pages_test.go / cbt_pages_test.go / cb7_pages_test.go, exercising the byte-limit eviction via ResetCBRPageCacheWithBytes / CBRPageCount and asserting via CBRPageCacheTotalBytes. If a test truly cannot reach a pageCache branch through any public API, that branch is dead code and should be removed (the public interface suffices).
[MAJOR] internal/files/cbr_pages.go:136, internal/files/cbt_pages.go:123, internal/files/cb7_pages.go:157 — on-skip read-error path does not account for uncompressed bytes in total budget
In doCBRLoad, doCBTLoad, doLoadCB7Pages: when a page read fails (the entry decompresses okay but reading its data errors), the entry is skipped via
continueWITHOUT accumulating its decompressed size into totalBytes. This creates a path where bytes can be unaccounted for if entries have readable headers but fail on actual I/O.Example (CBR, line 136):
if h.UnPackedSize > maxCBRPageBytes {
return nil, fmt.Errorf(...)
}
if totalBytes+h.UnPackedSize > maxCBRTotalBytes {
return nil, fmt.Errorf(...) // budget check passes
}
data, readErr := io.ReadAll(...)
if readErr != nil {
logger.Warn("skipping unreadable entry", ...)
continue // skipped, but totalBytes NOT incremented — inconsistent!
}
totalBytes += int64(len(data))
The uncompressed size is already validated and known; if the entry is unreadable, the bytes should either be:
(A) Tallied anyway (the uncompressed size was allocated logically),
(B) Not read in the first place (fail early before the header-known stage), or
(C) Treated as a hard error, not skipped.
This is not a catastrophic correctness bug under normal operation (read succeeds), but it is a latent edge case: an archive with many unreadable-but-valid entries could accumulate unaccounted bytes and bypass the budget guard.
Why it matters: The byte budget's purpose is to prevent unbounded memory allocation. Unaccounted bytes are a violation of that invariant, even if rare.
Fix: Choose one of (A), (B), (C). Recommend (A) — after
if readErr != niland beforecontinue, dototalBytes += int64(len(data))ortotalBytes += h.UnPackedSize(depending on whether you want actual-read bytes or declared-uncompressed bytes). This maintains the budget invariant.[MINOR] internal/files/page_cache_internal_test.go +38–+50 — byteLimit clamping test calls unexported newPageCache
The test at lines +38–+50 directly invokes unexported
newPageCache[int](4, 0, func([]int) int64 { return 0 })to verify the constructor clamps byteLimit to 1. This is white-box test code. Will be resolved by the fix for the BLOCKER above (convert to black-box via export_test.go helpers).Correctness verification (non-review):
✅ Per-load 1 GiB budget is enforced BEFORE decompression — the checks
totalBytes+h.UnPackedSize > maxCBRTotalBytesoccur beforeio.ReadAll(), so no unbounded pre-buffering occurs mid-read.✅ Page cache evicts by both count (256 archives) AND bytes (512 MiB default), with byte limits tuneable via export_test.go helpers (ResetCBRPageCacheWithBytes, etc.).
✅ Over-budget errors are domain sentinels (ErrCBRTotalBytesTooLarge, ErrCBTTotalBytesTooLarge, ErrCB7TotalBytesTooLarge), no workflow-engine import, correctly returned as permanent errors (no retry on these).
✅ Black-box tests (cbr_pages_test.go, cbt_pages_test.go, cb7_pages_test.go) verify byte-budget enforcement via public APIs (CBRPageCount, etc.) + ResetCBRPageCacheWithBytes, asserting cache eviction via CBRPageCacheLen().
✅ Sentinel errors are correctly exported in export_test.go (ErrCBRTotalBytesTooLargeExport, etc.) for black-box test assertions.
REVIEW VERDICT: 1 blocker, 1 major, 1 minor
Security Review — PR #1240 (ComicInfo.xml → exact-by-ID provider fetch)
Adversarial focus: ComicInfo.xml is attacker-controlled (inside an untrusted .cbz/.cbr).
SSRF (untrusted
<Web>/<Notes>steering outbound host) — CLEAN. The rawURL is never used as a request target.
extractProviderIDFromComicInfo(
internal/bookdrop/comicinfo_provider_id.go) only regex-captures a numeric issueID (
4000-(\d+),metron\.cloud/issue/(\d+),\[ComicVine:4000-(\d+)\]). Bothproviders build the request from the FIXED provider base URL:
fmt.Sprintf("%s/issue/4000-%d/?%s", baseURL, issueID, ...)(internal/metadata/comicvine/search.go:778),baseURL=defaultBaseURL/test server.fmt.Sprintf("%s/issue/%d/", d.baseURL, issueID)(internal/metadata/metron/provider.go:425).The persisted
WebLinkalso passes throughurlutil.SafeURL(internal/comic/persist.go:163).Injection / request-splitting — CLEAN. The ID is parsed with
strconv.ParseInt(match[1], 10, 64)and rejected unless> 0; it is emitted as%d. No non-numeric content can reach the path/query. Provider tag is a fixedliteral (
"comicvine"/"metron"), never concatenated into a URL.XXE / XML bomb — CLEAN. Parsing uses Go
encoding/xml(decodeComicInfoReader,internal/comic/comicinfo.go:258) with noCharsetReaderand no customEntitymap — external/DTD entities are not resolved by default. Decompressed size is
double-bounded: the zip/rar header uncompressed-size guard rejects
> maxComicInfoBytes(1 MB) before open, and anio.LimitReader(rc, cap)caps thedecode. Archive read is
io.LimitReader(f, 512MB). Zip entry is read, notextracted to disk (no zip-slip).
API keys — CLEAN. New log lines emit only
issue_id/volume_id/err(
internal/metadata/comicvine/provider.go); noapi_keyin the new code.Permanent vs transient — CORRECT. A malformed/missing/unsupported archive →
readComicArchiveerror/ErrNoComicInfois swallowed and falls back to fuzzysearch (
fetchMergedCandidateForProposal). A provider 404 for anattacker-forged ID maps to
metadata.ErrNoMatch(fall through to otherproviders), not a hard error. Exact-by-ID fetch still runs under the existing
rate limiter and only for already-enabled/active providers.
[MINOR] internal/bookdrop/import_metadata_service.go:15 — import out of goimports order
internal/comicis placed afterinternal/db/sqlc(comic < db/sqlc). Ifgolangci-lint/gci is not flagging it, harmless, but reorder for tidiness. No
security impact.
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Security Review — PR #1250 (CBR/CB7/CBT total-decompressed-byte budget)
Adversarial focus: incremental enforcement, byte-accounting integrity/overflow, cache-budget gaming, panic surface on malformed input.
[MAJOR] internal/files/cbr_pages.go:131 — total-byte budget bypassable by RAR unknown-size (UnPackedSize == -1) entries; the very OOM the PR targets remains reachable for CBR
The only budget guard is the pre-read check
if totalBytes+h.UnPackedSize > maxCBRTotalBytes. rardecode/v2 setsh.UnPackedSize = -1for any entry whose size is flagged unknown (file5UnpSizeUnknown / RAR1.5 unknown-size — a normal, trivially attacker-set flag, not an overflow). For such entries the pre-check evaluatestotalBytes + (-1) > limit, which decreases the running sum and never fires. The actual readio.ReadAll(io.LimitReader(r, maxCBRPageBytes))still buffers up to 50 MiB, andtotalBytes += len(data)records real bytes — but that accumulated real total is NEVER compared against maxCBRTotalBytes on its own; the limit is only ever tested astotalBytes + declared. So an archive of N image entries, each flagged unknown-size and delivering 50 MiB, buffers N x 50 MiB with the budget never tripping. N is bounded only by maxCBREntriesToScan = 5000, i.e. worst case ~250 GiB resident in one request — exactly the per-request OOM this PR claims to prevent. (Same class of bypass for CB7 via a UncompressedSize >= 2^63 that casts to a negative int64, cb7_pages.go:156; harder to trigger there because it needs an implausible declared size, but the guard has the identical structural gap. CBT is safe: archive/tar rejects negative h.Size.)Fix: enforce the budget against the accumulated real bytes, not the attacker-declared size. After
totalBytes += int64(len(data))addif totalBytes > maxCBRTotalBytes { return nil, fmt.Errorf("...: %w", ErrCBRTotalBytesTooLarge) }(and the CB7/CBT analogs). Optionally keep the pre-read check as a fast-path, but clamp the declared value to [0, maxPageBytes] first (sz := h.UnPackedSize; if sz < 0 || sz > maxCBRPageBytes { sz = maxCBRPageBytes }) so a negative/bogus header cannot subtract from or skip past the running total. This makes enforcement genuinely incremental and closes the -1 / overflow path.[MINOR] internal/files/page_cache.go:76 — a single archive can pin the cache above its byte budget
evictLocked stops evicting once c.ll.Len() == 0, so the incoming entry is always admitted even if it alone exceeds byteLimit. One archive can therefore hold the cache above the 512 MiB defaultPageCacheBytes (up to the per-load total cap). This is inherent to admit-current-entry LRU and bounded once the MAJOR above is fixed (per-load cap becomes real), so it is not blocking — but worth a comment noting the cache can transiently exceed byteLimit by up to one max-entry.
No further issues: cache byte-accounting is integer-correct (update path
totalBytes += entryBytes - old.bytes; eviction subtracts ev.bytes; sizeOf sums non-negative len(data)), the ll.Back() deref is nil-guarded by the ll.Len() > 0 loop condition, and no new panic surface is introduced on malformed input (all decode/read errors are handled, not paniced). Per-entry 50 MiB LimitReader cap remains intact.REVIEW VERDICT: 0 blocker, 1 major, 1 minor
4fde3b356f4f01cbe556[RE-REVIEW] byte-budget and byte-tracking fixes verified
BLOCKER (white-box test migration): FIXED
package filesbut contains ONLY refactored existing tests (extracting intCacheNoByteLimit helper)package files_testMAJOR 1 (byte-budget bypassable via UnPackedSize=-1 / uint64≥2^63): FIXED
MAJOR 2 (skipped-entry bytes untallied): FIXED
Code metrics: All helpers are <20 lines each, no gate violations.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor — APPROVED FOR MERGE
Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR)
Verified
git diff origin/main...origin/bd-bookshelf-t582g.7.The concurrent-accept clobbering MAJOR is CLOSED. Finalize now uses a non-clobbering
os.Link(failsEEXISTwhen the dest name already exists —os.Linkdoes not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (finalizeWithRetry, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser getsEEXISTand advances to the next(N)suffix. Confirmed viainternal/app/build_extended_deps.go:1806wiringLinkFile: os.Linkand the EEXIST-retry test (review_service_test.go:2051-2110).Checklist against the four points:
os.Link+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten.os.CreateTemp(destDir,...)inside the confineddestDir; every link candidate isfilepath.Join(destDir, base+"(N)"+ext)off afilepath.Base'd name with a numeric suffix. No separators, no.., no follow-through.[MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference
The TX records
FileName: safeFileName/FileSubPath: fileSubPath(review_service.go:955-957), computed byuniquifyDestPathBEFORE the TX (line 876).finalizeWithRetryruns AFTER the commit (line 998) and, onEEXIST, advances the on-disk path to a different suffix (e.g. DB saysnovel.epubbut the file links tonovel (2).epub— exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committedbook_filerow points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned fromfinalizeWithRetryand reconcile thebook_filerow (updatefile_name/file_sub_path) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbingupsertBookFileas a no-op — add an assertion that the storedFileNameequals the finally-linked name in the EEXIST-retry case.[MINOR] internal/bookdrop/review_service.go:997 —
LinkFilehas no nil-default despite the doc claiming oneThe doc comment (review_service.go:272) says "Defaults to os.Link when nil," but
linkFn := p.LinkFileis passed straight intofinalizeWithRetrywith noif linkFn == nil { linkFn = os.Link }guard (contrastStatFile/MkdirAll, which are defaulted). Production wiring passesos.Linkso this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract.REVIEW VERDICT: 0 blocker, 1 major, 1 minor
Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR)
Verified
git diff origin/main...origin/bd-bookshelf-t582g.7.The concurrent-accept clobbering MAJOR is CLOSED. Finalize now uses a non-clobbering
os.Link(failsEEXISTwhen the dest name already exists —os.Linkdoes not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (finalizeWithRetry, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser getsEEXISTand advances to the next(N)suffix. Confirmed viainternal/app/build_extended_deps.go:1806wiringLinkFile: os.Linkand the EEXIST-retry test (review_service_test.go:2051-2110).Checklist against the four points:
os.Link+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten.os.CreateTemp(destDir,...)inside the confineddestDir; every link candidate isfilepath.Join(destDir, base+"(N)"+ext)off afilepath.Base'd name with a numeric suffix. No separators, no.., no follow-through.[MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference
The TX records
FileName: safeFileName/FileSubPath: fileSubPath(review_service.go:955-957), computed byuniquifyDestPathBEFORE the TX (line 876).finalizeWithRetryruns AFTER the commit (line 998) and, onEEXIST, advances the on-disk path to a different suffix (e.g. DB saysnovel.epubbut the file links tonovel (2).epub— exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committedbook_filerow points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned fromfinalizeWithRetryand reconcile thebook_filerow (updatefile_name/file_sub_path) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbingupsertBookFileas a no-op — add an assertion that the storedFileNameequals the finally-linked name in the EEXIST-retry case.[MINOR] internal/bookdrop/review_service.go:997 —
LinkFilehas no nil-default despite the doc claiming oneThe doc comment (review_service.go:272) says "Defaults to os.Link when nil," but
linkFn := p.LinkFileis passed straight intofinalizeWithRetrywith noif linkFn == nil { linkFn = os.Link }guard (contrastStatFile/MkdirAll, which are defaulted). Production wiring passesos.Linkso this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract.REVIEW VERDICT: 0 blocker, 1 major, 1 minor
Security RE-REVIEW — PR #1250 (comic-reader OOM budget-bypass)
Scope: verify the RAR/CB7/CBT decompressed-bytes OOM budget-bypass MAJOR is closed.
Reviewed
git diff origin/main...origin/bd-bookshelf-t582g.10.Verdict on the tracked MAJOR: CLOSED. All four required guarantees hold.
(1) Accumulated-real-bytes guard rejects BEFORE RAM exhaustion — CONFIRMED.
internal/files/cbr_pages.goscanCBREntries(and the CB7/CBT equivalents) now:(a) pre-read
checkCB*EntryBudgetwith the declared size clamped, then(b) read each entry through
io.ReadAll(io.LimitReader(r, maxCB*PageBytes))(50 MiB per-entry cap), then (c) after each read do
totalBytes += int64(len(data))followed byif totalBytes > maxCB*TotalBytes(1 GiB) → return
Err*TotalBytesTooLarge. Worst-case peak is bounded at~1 GiB + one 50 MiB entry, and the check fires per-entry (not after buffering the
whole archive), so an archive of many unknown-size (-1) entries is rejected mid-scan
rather than after buffering ~250 GiB.
(2) Declared-size clamp handles -1 and uint64>=2^63 — CONFIRMED.
checkCBREntryBudget(cbr_pages.go):declared < 0 -> 0handles rardecode'sUnPackedSize == -1unknown-size flag; the pre-readtotalBytes + declarednolonger under-counts.
checkCB7EntryBudget(cb7_pages.go):declared := int64(rawSize); if declared < 0 { declared = 0 }correctly handles auint64 UncompressedSize >= 2^63thatcasts to a negative int64. Both are exercised by tests
(cbr_pages_test.go:499, cb7_pages_test.go:485).
(3) No remaining path buffers unbounded bytes without counting — CONFIRMED.
The only whole-buffer path is CB7's archive read
(
io.ReadAll(io.LimitReader(rc, maxCB7ArchiveBytes+1)), cb7_pages.go), capped at1 GiB+1 and rejected with
ErrCB7ArchiveTooLargebefore any parse. CBR/CBT streamentry-by-entry. The process-level
pageCachealso gains a byte-based eviction bound(
defaultPageCacheBytes = 512 MiB;evictLockedaccounts for the incoming entry),so cross-request cache growth is bounded too.
(4) No integer overflow in byte accounting — CONFIRMED.
All accumulators are
int64; per-entry additions are <= 50 MiB andtotalBytestops out at ~1.05 GiB before rejection — nowhere near 2^63. CBT's pre-read
totalBytes + sizeis only reached for non-oversizedsize(a size near 2^63 isrejected first by
size > maxCBTPageBytes); negative-sizecases fall through tothe post-read
len(data)guard. No dangerous wrap.Architecture boundary — CONFIRMED clean.
internal/filesimports noworkflow-engine package; the new
Err*TotalBytesTooLargeare plain sentinels. Thepage loaders are wired only into HTTP page-serving handlers (
internal/books/wire.go),not a workflow activity, so no
NewPermanentErrorclassification is owed here.Findings
[MINOR] internal/files/page_cache.go:122 —
loadOrStorehas no production caller.loadOrStore(and thetotalCachedBytesaccessor) are referenced only by thepre-existing
page_cache_internal_test.gowhite-box test; the CBR/CB7/CBT loadersuse
get/add+ singleflight instead. Not introduced by this PR and not asecurity issue, but it is dead production code kept alive only by a white-box test.
Suggest deleting
loadOrStore(and its dedicated internal-test specs), or wiringit in to replace the get/add+singleflight duplication. Pre-existing; noted for
cleanup, not a blocker for this fix.
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
4f01cbe556dd89b06294