feat(manifest): faster, correct, and resilient manifest path resolution - #833
feat(manifest): faster, correct, and resilient manifest path resolution#833vilenarios wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #833 +/- ##
===========================================
+ Coverage 79.43% 79.49% +0.05%
===========================================
Files 138 139 +1
Lines 53401 53682 +281
Branches 4087 4119 +32
===========================================
+ Hits 42420 42673 +253
- Misses 10928 10956 +28
Partials 53 53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vilenarios
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughThe change validates manifest IDs, corrects index precedence, adds bounded in-memory caching, persists root resolutions in SQLite, and wires the resolver to configuration and the shared database store. ChangesManifest resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/database/standalone-sqlite.ts`:
- Around line 4454-4461: Wrap the getManifestResolution case in its own braces
so the manifestResolution declaration is scoped only to that case and no longer
violates noSwitchDeclarations. Keep the existing worker.getManifestResolution
call, postMessage behavior, and break unchanged.
In `@src/lib/encoding.ts`:
- Around line 446-450: Update the fallback handling in the streaming resolver to
store a valid fallback candidate instead of resolving immediately in the
fallback listener. In the end handler, resolve that candidate only after index
and matching path events have had the opportunity to resolve; preserve the
existing precedence for index or requested matching paths and add fixtures
covering fallback-before-index and fallback-before-matching-path.
In `@src/resolution/streaming-manifest-path-resolver.ts`:
- Around line 95-120: Update queueWork to handle the promise returned by the
finalizer attached with ret.finally(...). Ensure SQLite wrapper rejection cannot
leave the derived finalizer promise unhandled by attaching a catch handler or
returning a handled derived promise, while preserving ret’s existing behavior.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 046c25bd-aee4-466e-ac9f-2f41a077cfb0
📒 Files selected for processing (18)
docker-compose.yamldocs/envs.mdmigrations/2026.08.04T12.00.00.data.init-manifest-resolutions.sqlmigrations/down/2026.08.04T12.00.00.data.init-manifest-resolutions.sqlsrc/config.tssrc/database/manifest-resolutions.test.tssrc/database/sql/data/manifest-resolutions.sqlsrc/database/standalone-sqlite.tssrc/lib/encoding.test.tssrc/lib/encoding.tssrc/resolution/streaming-manifest-path-resolver.test.tssrc/resolution/streaming-manifest-path-resolver.tssrc/system.tssrc/types.d.tstest/data-schema.sqltest/mock_files/manifests/example_manifest_invalid_ids.jsontest/mock_files/manifests/example_manifest_v0.2.0_paths_before_index_path_first.jsontest/stubs.ts
| case 'getManifestResolution': | ||
| const manifestResolution = worker.getManifestResolution(args[0]); | ||
| parentPort?.postMessage(manifestResolution); | ||
| break; | ||
| case 'saveManifestResolution': | ||
| worker.saveManifestResolution(args[0]); | ||
| parentPort?.postMessage(null); | ||
| break; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Scope manifestResolution to this case.
Line 4455 declares manifestResolution in the switch scope. Biome reports lint/correctness/noSwitchDeclarations as an error. Wrap this case in braces so the lint check passes.
Proposed fix
- case 'getManifestResolution':
+ case 'getManifestResolution': {
const manifestResolution = worker.getManifestResolution(args[0]);
parentPort?.postMessage(manifestResolution);
break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'getManifestResolution': | |
| const manifestResolution = worker.getManifestResolution(args[0]); | |
| parentPort?.postMessage(manifestResolution); | |
| break; | |
| case 'saveManifestResolution': | |
| worker.saveManifestResolution(args[0]); | |
| parentPort?.postMessage(null); | |
| break; | |
| case 'getManifestResolution': { | |
| const manifestResolution = worker.getManifestResolution(args[0]); | |
| parentPort?.postMessage(manifestResolution); | |
| break; | |
| } | |
| case 'saveManifestResolution': | |
| worker.saveManifestResolution(args[0]); | |
| parentPort?.postMessage(null); | |
| break; |
🧰 Tools
🪛 Biome (2.5.6)
[error] 4455-4455: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for 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.
In `@src/database/standalone-sqlite.ts` around lines 4454 - 4461, Wrap the
getManifestResolution case in its own braces so the manifestResolution
declaration is scoped only to that case and no longer violates
noSwitchDeclarations. Keep the existing worker.getManifestResolution call,
postMessage behavior, and break unchanged.
Source: Linters/SAST tools
Addresses CodeRabbit review on #833. queueWork attached `ret.finally(...)` for metrics but returned the bare `ret`, leaving the finally-derived promise orphaned — on rejection it would surface as an unhandledRejection independent of the caller's own handler. Returning the `.finally()` chain delivers the rejection to the caller (settlement passes through unchanged, so behavior is preserved). Newly relevant because the manifest resolution store persists via a fire-and-forget write. Also adds a fallback-declared-first manifest fixture + tests proving that a fallback appearing before the index/paths in the JSON never preempts the index or a matching path (the 'fallback' event is emitted only at stream end) — documenting the precedence CodeRabbit asked about. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
|
Thanks @coderabbitai — addressed in d964bd5: 1. 2. Fallback resolution ordering — verified as already-correct, and added a proof. 3. |
|
Tip For best results, initiate chat on the files or code changes.
The fallback ordering concern does not apply. Keeping the unbraced 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Two correctness fixes in the streaming manifest parser (resolveManifestStreamPath): - Defer index resolution until the `index` object closes so `index.id` deterministically wins over `index.path`, regardless of JSON key order. Previously a v0.2.0 manifest whose `paths` preceded `index` (with `path` before `id` inside `index`) resolved to the id `index.path` mapped to, not `index.id` — so the served index could differ from other gateways. - Reject data ids that are not 43-char base64url before resolving them, avoiding data-retrieval attempts on malformed manifest entries. Adds regression fixtures/tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
…uests A manifest transaction is immutable, so a resolved (manifest id, path) -> data id mapping is valid forever. StreamingManifestPathResolver now keeps a bounded LRU of resolutions: resolveFromData populates it, resolveFromIndex serves hits with `complete: true` so the caller skips fetching and re-parsing the manifest body entirely. Every ArNS site is a manifest, and each asset on a page is a separate path resolution against the same manifest, so the current code re-parses the whole body per asset. On a real 7,784-path (500 KB) manifest, resolution drops from ~57 ms to a sub-ms cache lookup (~190x on repeat). Size configurable via MANIFEST_RESOLUTION_CACHE_SIZE (default 5000). Positive and negative resolutions are both cached (immutable per tx). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
Addresses CodeRabbit review on #832. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
…parsing Implements the previously-stubbed resolveFromIndex against a persistent, lazily-populated store, so a manifest's root/index survives restarts and can be served even when the manifest body is momentarily unreachable upstream. - New `manifest_resolutions` table (data.db) keyed by manifest tx id, storing index_id / fallback_id. Dedicated table — no stable_* / auto-verify / flush entanglement. Migration is additive with a reversing down migration. - New DB methods getManifestResolution (read) / saveManifestResolution (upsert, COALESCE-preserving) on the 'data' worker pool, exposed via the ManifestResolutionStore interface. - StreamingManifestPathResolver now consults the store for the root only (index/fallback — the highest-traffic case), and lazily persists the root resolution after resolveFromData. Sub-paths are never stored (the index has no path map) and continue to be served from the in-memory cache (PR #832). Persistence is fire-and-forget and store failures never break serving. Values are immutable per manifest transaction, so no invalidation is needed. Tests: real-SQLite round-trip incl. COALESCE upsert; resolver store hits for index/fallback, sub-path skip, lazy persist, store-failure fallthrough, store-optional. standalone-sqlite suite unaffected (69/69). Stacked on #832 (feat/manifest-resolution-improvements). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
Addresses CodeRabbit review on #833. queueWork attached `ret.finally(...)` for metrics but returned the bare `ret`, leaving the finally-derived promise orphaned — on rejection it would surface as an unhandledRejection independent of the caller's own handler. Returning the `.finally()` chain delivers the rejection to the caller (settlement passes through unchanged, so behavior is preserved). Newly relevant because the manifest resolution store persists via a fire-and-forget write. Also adds a fallback-declared-first manifest fixture + tests proving that a fallback appearing before the index/paths in the JSON never preempts the index or a matching path (the 'fallback' event is emitted only at stream end) — documenting the precedence CodeRabbit asked about. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9
d964bd5 to
59ce2ab
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/resolution/streaming-manifest-path-resolver.test.ts (3)
34-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd TSDoc for
FakeStore.Document that
FakeStoreis an in-memoryManifestResolutionStoretest double and that it records reads and writes. As per coding guidelines, "Add or improve TSDoc comments on code you touch."🤖 Prompt for 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. In `@src/resolution/streaming-manifest-path-resolver.test.ts` around lines 34 - 61, Add a TSDoc comment immediately above the FakeStore class describing it as an in-memory ManifestResolutionStore test double that records read and write operations through getCalls and saved. Keep the implementation unchanged.Source: Coding guidelines
64-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest cache eviction at the configured capacity.
The tests verify cache hits but do not verify that
cacheSizeevicts older entries. Construct the resolver withcacheSize: 1, resolve two distinct keys, and assert that the first key returns a cache miss.🤖 Prompt for 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. In `@src/resolution/streaming-manifest-path-resolver.test.ts` around lines 64 - 143, Add a test in the “resolveFromIndex caching” suite that constructs StreamingManifestPathResolver with cacheSize: 1, resolves two distinct manifest/path keys through resolveFromData, then calls resolveFromIndex for the first key and asserts it returns a cache miss with complete false. Keep the existing cache-hit assertions unchanged.
179-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining persistence write paths.
The tests verify a successful root index write only. Add a root fallback test that asserts
fallbackIdis saved. Add a rejectingsaveManifestResolutiontest that verifies the rejected fire-and-forget write does not create an unhandled rejection.🤖 Prompt for 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. In `@src/resolution/streaming-manifest-path-resolver.test.ts` around lines 179 - 210, Extend the persistence coverage around StreamingManifestPathResolver: add a root-resolution test that exercises the manifest fallback and asserts the saved record contains the expected fallbackId, then add a test using a rejecting saveManifestResolution implementation to verify the fire-and-forget persistence failure is handled without an unhandled rejection. Reuse the existing FakeStore, resolver setup, and root/sub-path distinction.src/database/standalone-sqlite.ts (1)
1814-1844: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd TSDoc for the manifest-resolution API.
Document the cache-miss result, optional ID semantics, and
resolvedAtunit.
src/database/standalone-sqlite.ts#L1814-L1844: Add TSDoc to the worker read and write methods.src/database/standalone-sqlite.ts#L4087-L4100: Add TSDoc to the public queue-wrapper methods.As per coding guidelines, “Add or improve TSDoc comments on code you touch”.
🤖 Prompt for 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. In `@src/database/standalone-sqlite.ts` around lines 1814 - 1844, Add TSDoc to getManifestResolution and saveManifestResolution in src/database/standalone-sqlite.ts:1814-1844, documenting that reads return undefined on cache misses, indexId and fallbackId are optional, and resolvedAt uses the expected time unit. Add corresponding TSDoc to the public queue-wrapper methods in src/database/standalone-sqlite.ts:4087-4100, preserving the same API semantics and parameter documentation.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/database/standalone-sqlite.ts`:
- Around line 1814-1844: Add TSDoc to getManifestResolution and
saveManifestResolution in src/database/standalone-sqlite.ts:1814-1844,
documenting that reads return undefined on cache misses, indexId and fallbackId
are optional, and resolvedAt uses the expected time unit. Add corresponding
TSDoc to the public queue-wrapper methods in
src/database/standalone-sqlite.ts:4087-4100, preserving the same API semantics
and parameter documentation.
In `@src/resolution/streaming-manifest-path-resolver.test.ts`:
- Around line 34-61: Add a TSDoc comment immediately above the FakeStore class
describing it as an in-memory ManifestResolutionStore test double that records
read and write operations through getCalls and saved. Keep the implementation
unchanged.
- Around line 64-143: Add a test in the “resolveFromIndex caching” suite that
constructs StreamingManifestPathResolver with cacheSize: 1, resolves two
distinct manifest/path keys through resolveFromData, then calls resolveFromIndex
for the first key and asserts it returns a cache miss with complete false. Keep
the existing cache-hit assertions unchanged.
- Around line 179-210: Extend the persistence coverage around
StreamingManifestPathResolver: add a root-resolution test that exercises the
manifest fallback and asserts the saved record contains the expected fallbackId,
then add a test using a rejecting saveManifestResolution implementation to
verify the fire-and-forget persistence failure is handled without an unhandled
rejection. Reuse the existing FakeStore, resolver setup, and root/sub-path
distinction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9f28a1b-1929-4d8e-bd20-e414ec416cb5
📒 Files selected for processing (19)
docker-compose.yamldocs/envs.mdmigrations/2026.08.04T12.00.00.data.init-manifest-resolutions.sqlmigrations/down/2026.08.04T12.00.00.data.init-manifest-resolutions.sqlsrc/config.tssrc/database/manifest-resolutions.test.tssrc/database/sql/data/manifest-resolutions.sqlsrc/database/standalone-sqlite.tssrc/lib/encoding.test.tssrc/lib/encoding.tssrc/resolution/streaming-manifest-path-resolver.test.tssrc/resolution/streaming-manifest-path-resolver.tssrc/system.tssrc/types.d.tstest/data-schema.sqltest/mock_files/manifests/example_manifest_invalid_ids.jsontest/mock_files/manifests/example_manifest_v0.2.0_fallback_first.jsontest/mock_files/manifests/example_manifest_v0.2.0_paths_before_index_path_first.jsontest/stubs.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- docker-compose.yaml
- src/types.d.ts
- src/database/sql/data/manifest-resolutions.sql
- migrations/2026.08.04T12.00.00.data.init-manifest-resolutions.sql
- test/stubs.ts
- test/mock_files/manifests/example_manifest_invalid_ids.json
- test/data-schema.sql
- src/database/manifest-resolutions.test.ts
- test/mock_files/manifests/example_manifest_v0.2.0_paths_before_index_path_first.json
- src/resolution/streaming-manifest-path-resolver.ts
- src/system.ts
- docs/envs.md
- src/lib/encoding.test.ts
- src/config.ts
- src/lib/encoding.ts
1 participant
Supersedes #832 — this PR now contains those commits (1a–1c) plus the persistent index (1d) as a single, self-contained change against
develop. No schema entanglement beyond one dedicated table.Every ArNS site is an Arweave manifest (URL paths → data ids), and each asset on a page is a separate path resolution against the same manifest. This PR fixes two correctness bugs, removes redundant re-parsing, and makes resolution survive restarts and upstream outages.
Changes
fix— deterministic v0.2.0 index precedence (1a). Index resolution is deferred until theindexobject closes, soindex.idwins overindex.pathregardless of JSON key order. Previously a manifest whosepathsprecededindex(withpathbeforeid) resolved to the wrong id — the served index could differ from other gateways. (Bug reproduced, then fixed; regression test added. Depth guard excludes apathsentry keyed"index".)fix— reject malformed ids (1b). Manifest entries whose id isn't 43-char base64url are rejected before a data-retrieval attempt.perf— in-memory resolution cache (1c). A manifest tx is immutable, so a resolved(manifest id, path) → data idmapping is valid forever.StreamingManifestPathResolverkeeps a bounded LRU; repeat resolutions skip fetch + parse. Size viaMANIFEST_RESOLUTION_CACHE_SIZE(default 5000).feat— persistent index (1d). Implements the previously-stubbedresolveFromIndexagainst a durable store: a newmanifest_resolutionstable (data.db, dedicated — nostable_*/auto-verify/flush/ClickHouse entanglement) keyed by manifest tx id →index_id/fallback_id, withgetManifestResolution/saveManifestResolutionon the data worker pool behind aManifestResolutionStoreinterface. The resolver consults the store for the root only (index/fallback — mirrors legacyhandleManifestFromDdb) and lazily persists on resolve. Fire-and-forget; store failures never break serving. Additive migration with a reversing down migration.Before/after (real manifests from a live gateway)
Repeat resolutions, body in memory (isolates parse cost):
Cold cache / after restart (persistent index), large manifest:
Tests
encoding.test.ts+4 (index-id precedence regression; malformed-id rejection) — 29/29.streaming-manifest-path-resolver.test.ts(new, 12): cache hit/miss/negative, trailing-slash keying, store hit for index/fallback, sub-path skip, lazy persist, store-failure fallthrough, store-optional.manifest-resolutions.test.ts(new, 4): real-SQLite round-trip incl. COALESCE upsert, missing-manifest.standalone-sqlite.test.ts69/69 (no worker regression). Lint clean; zero typecheck errors in changed files.Migration
migrations/2026.08.04T12.00.00.data.init-manifest-resolutions.sql(+down/).test/data-schema.sqlregenerated (only the new table).Behavioral notes for review
Not in this PR
1eobservability metrics (small follow-up). Eager population at unbundle time deliberately deferred (lazy-on-request covers L1 + bundled uniformly).New env var
MANIFEST_RESOLUTION_CACHE_SIZEdocumented indocs/envs.md+docker-compose.yaml.🤖 Generated with Claude Code
https://claude.ai/code/session_01LPbcF2M3P1XcM8mU8FWEf9