perf(pages): reuse bounded immutable payload reads#87
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds bounded LRU reuse for immutable ChangesBounded page payload reuse
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PageService
participant BoundedPromiseCache
participant Persistence
Caller->>PageService: pages.get(handle, maxBytes)
PageService->>BoundedPromiseCache: getOrCreate(root.oid, read)
BoundedPromiseCache->>Persistence: readBlobStream(root)
Persistence-->>BoundedPromiseCache: payload bytes
BoundedPromiseCache-->>PageService: cached payload
PageService-->>Caller: copied bytes
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
@codex Self-Code Review / Code Lawyer report
Resolved findings
Open findingsNone. The implementation preserves immutable OID identity, per-call root/size validation, caller-copy isolation, failure retry, bounded completed residency, streaming Evidence
Please independently confirm the conclusions and flag any missed contract or lifecycle issue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/0051-bounded-page-payload-reuse/bounded-page-payload-reuse.md`:
- Around line 37-38: Update the retained-property fixture sentence to hyphenate
the compound modifier, changing “1.80 second time” to “1.80-second time” while
preserving the surrounding wording and measurements.
- Around line 44-49: Update the bounded cache eviction logic in
boundedPromiseCache so pending promise entries are never evicted when maxEntries
is exceeded; evict only settled entries, or track in-flight work separately
while preserving bounded completed-entry storage. Ensure repeated requests for
an evicted-by-capacity OID still share its existing in-flight promise, and add a
regression test covering more concurrent unique OIDs than pageCacheEntries.
In `@src/domain/services/PageService.js`:
- Around line 90-93: The bounded cache must not evict pending promises, since
doing so breaks read coalescing and can displace resident entries before weight
is known. Update the cache used by PageService’s `#payloads/getOrCreate` flow to
retain in-flight reads separately or restrict eviction to completed entries,
while preserving bounded LRU behavior for completed payloads; add a contention
test covering repeated A/B/A reads and an oversized pending read.
In `@test/integration/bundle-reference-performance.test.js`:
- Around line 120-124: Update PageService.get() to avoid resolveRoot() metadata
reads on cached page hits: either cache and reuse the validated size/type
alongside the payload, or perform the limit check before
readObjectType/readObjectSize. Preserve validation for cold or uncached reads
while ensuring warm reads produce no Git metadata I/O.
In `@test/unit/domain/services/PageService.test.js`:
- Around line 94-144: Expand the PageService cache tests to exercise true LRU
ordering and aggregate byte eviction: change the entry-count scenario to use at
least two entries and access items so the least-recently-used payload is the one
reread, then add a positive pageCacheBytes case where multiple retained payloads
exceed the total budget and verify the expected rereads through
persistence.readBlobStream. Keep the existing zero-byte and oversized-payload
coverage intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c20f433a-5576-4bfc-b6b9-56d77476390e
📒 Files selected for processing (13)
CHANGELOG.mdGUIDE.mddocs/API.mddocs/design/0051-bounded-page-payload-reuse/bounded-page-payload-reuse.mddocs/design/README.mddocs/releases/v6.5.1.mdindex.d.tsindex.jssrc/domain/services/PageService.jstest/integration/bundle-reference-performance.test.jstest/unit/domain/services/PageService.test.jstest/unit/facade/ContentAddressableStore.application-storage.test.jstest/unit/types/declaration-accuracy.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test-unit
🧰 Additional context used
📓 Path-based instructions (3)
docs/design/**
📄 CodeRabbit inference engine (AGENTS.md)
Use
docs/design/directory for durable design contracts and proof plans
Files:
docs/design/README.mddocs/design/0051-bounded-page-payload-reuse/bounded-page-payload-reuse.md
GUIDE.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
GUIDE.mdfor orientation and productive-fast path documentation
Files:
GUIDE.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
CHANGELOG.mdto record the historical truth of merged behavior
Files:
CHANGELOG.md
🧠 Learnings (3)
📚 Learning: 2026-02-28T19:21:13.982Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 15
File: test/unit/domain/services/CasService.envelope.test.js:326-330
Timestamp: 2026-02-28T19:21:13.982Z
Learning: In fuzz tests for cryptographic operations, use a seeded PRNG (e.g., xorshift32) for control-flow variables such as plaintext size selection and recipient index selection to ensure reproducibility. Continue to use randomBytes() for cryptographic keys and nonces to preserve realistic randomness and avoid security anti-patterns. This guideline applies to test files that perform fuzz testing of crypto logic; implement a consistent seed setup (e.g., fixed seed in test initialization) and document the rationale to enable deterministic replays across runs.
Applied to files:
test/unit/facade/ContentAddressableStore.application-storage.test.jstest/unit/types/declaration-accuracy.test.jstest/integration/bundle-reference-performance.test.jstest/unit/domain/services/PageService.test.js
📚 Learning: 2026-03-30T19:53:48.000Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 29
File: docs/design/TR-010-planning-index-consistency-review.md:121-131
Timestamp: 2026-03-30T19:53:48.000Z
Learning: In this repo, CHANGELOG.md entries should be user-facing and descriptive (bullet points) rather than cycle-ID headings (e.g., use a phrase like “Planning-index consistency review” instead of a “TR-010 — …” heading). When searching the changelog, don’t rely on grepping for cycle IDs (e.g., “TR-010”) because it may cause false negatives; search for the descriptive keywords/phrases from the bullet entries instead.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-13T17:00:46.222Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 65
File: src/domain/helpers/isCanonicalCollectionKey.js:21-35
Timestamp: 2026-07-13T17:00:46.222Z
Learning: In git-stunts/git-cas, the codebase is intended to run on multiple JavaScript runtimes (Node, Bun, and Deno), not just Node. During code review, don’t suggest replacing existing portable helper logic with newer Node-only built-ins (e.g., Node 20+ APIs like `String.prototype.isWellFormed()`) unless the change preserves Bun/Deno compatibility (via portable/polyfill implementations or safe runtime gating). If a file includes manual implementations (such as `src/domain/helpers/isCanonicalCollectionKey.js`), treat them as intentional to avoid requiring a higher host built-in baseline.
Applied to files:
src/domain/services/PageService.js
🪛 LanguageTool
docs/design/0051-bounded-page-payload-reuse/bounded-page-payload-reuse.md
[grammar] ~38-~38: Use a hyphen to join words.
Context: ...onds, including 32 blob reads and a 1.80 second time to first reading. - Page OID...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (10)
index.d.ts (1)
709-712: LGTM!index.js (1)
136-137: LGTM!Also applies to: 158-159, 180-181, 240-240, 378-379, 631-633, 658-660, 682-684
docs/design/0051-bounded-page-payload-reuse/bounded-page-payload-reuse.md (1)
1-36: LGTM!Also applies to: 39-43, 50-71, 76-143
test/unit/domain/services/PageService.test.js (1)
1-14: LGTM!Also applies to: 75-92, 147-172
test/unit/facade/ContentAddressableStore.application-storage.test.js (1)
75-86: LGTM!test/unit/types/declaration-accuracy.test.js (1)
116-117: LGTM!src/domain/services/PageService.js (2)
7-11: LGTM!Also applies to: 20-47, 136-146, 204-210
85-90: 🚀 Performance & ScalabilityVerify that warm reads avoid repeated Git metadata work.
get()callsresolveRoot()before consulting#payloads;resolveRoot()invokes object-type and size reads on every call. Unless those persistence operations are independently cached, an identical warmpages.get()can still issue Git-backed metadata commands, conflicting with the zero-additional-command objective. Confirm this with the real-Git command-count test and add a validated metadata fast path if needed.docs/design/README.md (1)
14-14: LGTM!docs/API.md (1)
83-84: LGTM!
|
@codex Self-Code Review / Code Lawyer re-review after
Review dispositionAll five CodeRabbit findings were valid and are addressed:
No additional correctness, lifecycle, compatibility, or documentation findings remain in the current diff. Evidence
Please independently confirm the conclusions and flag any missed contract or lifecycle issue. |
1 participant
Summary
pages.get()payload reads by immutable page OIDpages.open()uncached, streaming, and outside retention authorityWhy
git-warpretained observations repeatedly opened the same immutable page payloads. Metadata reuse from v6.5.0 removed part of the Git command amplification, but everypages.get()still started another blob read. The cache belongs ingit-cas, which owns immutable CAS access and residency policy.Compatibility
This changes no persisted format, handle, ref, witness, or existing method signature.
pageCacheEntriesandpageCacheBytesare additive constructor options. Cache residence is not retention evidence.Verification
pnpm run release:verify: 14/14 steps, 6,652 observed tests across Node, Bun, and Denopnpm test: 2,029 passed, 2 skipped after review hardeningpnpm run lint: passedFollow-up
Path-local persistent bundle derivation remains separate in #86.
Closes #85