Add progressive subtree caching and singleflight to IPFS tree resolution#92
Add progressive subtree caching and singleflight to IPFS tree resolution#92hubsmoke wants to merge 1 commit into
Conversation
…sal, and progressive caching Three root causes of the myst query fragility: 1. Concurrent requests for the same tree spawn duplicate worker pools 2. BFS traversal prevents caching until the entire tree resolves 3. Only caching fully-resolved trees means timed-out requests make zero progress Changes: - Singleflight deduplication: concurrent requests for the same tree share a single resolution promise instead of spawning independent worker pools - DFS traversal (LIFO stack): subtrees complete before sibling branches, enabling progressive caching of completed portions - Subtree completion tracking: each directory tracks pending children; when a subtree fully resolves, it's cached individually by CID - Subtree grafting: on retry, workers check for previously-cached subtrees and graft them directly, skipping re-traversal - Per-CID DAG node caching: individual fetchDagNode responses are cached in Redis (4h TTL), making any retry cheaper at the node level - Subtree cleanup: when the full tree is successfully cached, redundant subtree entries are deleted from Redis https://claude.ai/code/session_01Fr42CdDyhXSJCU4b7CtFNt
📝 WalkthroughWalkthroughThe pull request introduces per-CID caching for DAG nodes and subtrees, implements a singleflight mechanism to coalesce concurrent tree resolutions, and refactors tree resolution to use depth-first search with progressive subtree caching. Additionally, a new Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Caller
participant SF as Singleflight<br/>(inflightTrees)
participant Cache as Redis Cache
participant Resolver as resolveIpfsTree<br/>(DFS Worker)
participant IPFS as IPFS/DAG<br/>Fetcher
Client->>SF: getIpfsFolderTreeByCid(cid)
alt In-flight resolution exists
SF-->>Client: Join existing promise
else New resolution
SF->>Resolver: Start DFS resolution
activate Resolver
Resolver->>Cache: Check subtree cache
alt Subtree cached
Cache-->>Resolver: Return cached subtree
Resolver->>Resolver: fixSubtreePaths(entry, basePath)
else Subtree not cached
Resolver->>IPFS: fetchDagNode(cid)
IPFS->>Cache: Check DAG node cache
alt DAG node cached
Cache-->>IPFS: Return cached node
else DAG node not cached
IPFS->>IPFS: Fetch from gateway/fallback
IPFS->>Cache: Cache DAG node result
end
IPFS-->>Resolver: DAG node data
Resolver->>Resolver: DFS traverse children<br/>(LIFO worker pool)
Resolver->>Cache: Cache completed subtree
end
Resolver->>Cache: Cleanup redundant entries
deactivate Resolver
Resolver-->>SF: Return full tree
end
SF-->>Client: Return IpfsEntry tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/v2/data/getIpfsFolder.ts (1)
472-490:⚠️ Potential issue | 🟡 MinorDirectory entries use parent's gateway instead of the gateway that fetched them.
Line 479 assigns
gateway: (item.parent as EnhancedIpfsEntry).gateway, but the directory's DAG node was fetched at line 471 and has its owngatewayproperty. This is inconsistent with file entries (line 498) which correctly usedagNode.gateway.If the parent was fetched from a fallback gateway but this directory's DAG was fetched from the primary gateway, the entry would incorrectly reflect the fallback.
🔧 Suggested fix
const dirEntry: EnhancedIpfsEntry = { name: item.linkName, path: item.path, cid: item.cid, type: "directory", children: [], - gateway: (item.parent as EnhancedIpfsEntry).gateway, + gateway: dagNode.gateway, };
🧹 Nitpick comments (2)
src/api/v2/data/getIpfsFolder.ts (2)
183-189: Silent error swallowing on cache writes is inconsistent with other cache operations.Cache write failures here are completely swallowed with
.catch(() => {}), while similar operations elsewhere (e.g., line 388, line 545) include logging. Consider adding a debug/warn log for consistency and to aid troubleshooting.🔧 Suggested improvement
const result = { ...data, gateway: chosenGateway } as EnhancedIpfsEntry; if (redisService) { void redisService .setToCache(getKeyForDagNode(arg), result, DAG_NODE_CACHE_TTL) - .catch(() => {}); + .catch((error) => { + logger.debug({ error, cid: arg }, "Failed to cache DAG node"); + }); } return result;Apply the same pattern at line 266:
if (redisService) { - void redisService.setToCache(getKeyForDagNode(arg), result, DAG_NODE_CACHE_TTL).catch(() => {}); + void redisService.setToCache(getKeyForDagNode(arg), result, DAG_NODE_CACHE_TTL).catch((error) => { + logger.debug({ error, cid: arg }, "Failed to cache DAG node from public gateway"); + }); }
375-398: The fallback?? 1in pending count lookup may mask bugs.Line 377 uses
(pendingChildren.get(parent) ?? 1) - 1, which would result in0if the parent was never registered. While the current code paths appear to always register parents before their children resolve, this defensive fallback could silently trigger premature completion if a bug is introduced later.Consider either asserting the parent exists (fail-fast), or logging when the fallback is used:
🛡️ Alternative: fail-fast or log on unexpected state
const onChildResolved = (parent: IpfsEntry, childHadError: boolean) => { if (childHadError) subtreeHasError.add(parent); - const remaining = (pendingChildren.get(parent) ?? 1) - 1; + const current = pendingChildren.get(parent); + if (current === undefined) { + logger.warn({ parentCid: parent.cid, parentPath: parent.path }, "onChildResolved called for untracked parent"); + return; + } + const remaining = current - 1; pendingChildren.set(parent, remaining);
3 participants
Summary
Improves IPFS tree resolution reliability and performance by introducing three complementary caching and concurrency strategies: per-CID DAG node caching, progressive subtree caching with DFS traversal, and singleflight to prevent concurrent duplicate requests.
Key Changes
Per-CID DAG node cache: Added caching for individual DAG nodes fetched via
fetchDagNode()with a 4-hour TTL. This avoids redundant IPFS HTTP calls when retries occur, improving resilience to transient failures.Progressive subtree caching: Implemented DFS (depth-first) tree traversal that completes subtrees before exploring sibling branches. Completed subtrees are cached individually, allowing partial progress to be preserved even if the full tree resolution times out. On retry, previously-resolved subtrees are grafted directly from cache rather than re-traversed.
Singleflight mechanism: Added
inflightTreesmap to coalesce concurrent requests for the same tree. If a request is already resolving a tree, subsequent requests join the in-flight promise instead of spawning duplicate worker sets. This prevents worker stacking on retries.Subtree completion tracking: Introduced
pendingChildrenandparentOfmaps to track when directory subtrees are fully resolved. Completion propagates upward, triggering subtree caching at each level.Path fixing for cached subtrees: Added
fixSubtreePaths()utility to adjust absolute paths when grafting cached subtrees at new positions in the tree (CIDs are content-addressed, so structure is identical but paths depend on mount location).Redis
del()method: ExtendedRedisServiceinterface with adel()method to clean up now-redundant subtree cache entries once the full tree is successfully cached.Implementation Details
pop()) to enforce DFS order.catch()) to prevent failures from blocking resolutionhttps://claude.ai/code/session_01Fr42CdDyhXSJCU4b7CtFNt
Summary by CodeRabbit