HDDS-15826. Recon: add cycle guard to NSSummary /du tree walks#10723
Draft
smengcl wants to merge 3 commits into
Draft
HDDS-15826. Recon: add cycle guard to NSSummary /du tree walks#10723smengcl wants to merge 3 commits into
smengcl wants to merge 3 commits into
Conversation
The disk-usage read path walks the NSSummary directory tree recursively in three places: EntityHandler.getTotalFileSizeDist, getTotalDirCount, and ReconUtils.gatherSubPaths. None guard against a corrupted tree in which a directory lists itself (or an ancestor) as a child, so such a cycle drives unbounded recursion and crashes Recon with a StackOverflowError (HTTP 500 on /du). Convert all three walks to iterative traversal with a visited-set cycle guard. Each directory is now visited and counted at most once, so a self-referencing or cyclic child terminates the walk with a sane result instead of overflowing the stack. Clean-tree results are unchanged. gatherSubPaths now fetches each node once (on pop) rather than twice. Because de-duplicating silently would hide the corruption from an observability tool, each walk logs a WARN (once per request, with the path/object context) the first time a repeated reference is detected. Tests: TestEntityHandlerCycleGuard covers self-referencing, cyclic, and clean trees for the two EntityHandler walks; TestReconUtils gains a cyclic-tree case for gatherSubPaths. All use mocks with a timeout, so they add negligible CI time. Follow-up (not in this commit): add response paging/limit to the /du endpoint to bound the response for very large buckets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Recon’s NSSummary tree-walk logic (used by /du, file-size distribution, directory counts, and open-file subpath gathering) against corrupted or pathologically deep directory graphs by replacing recursive traversal with an iterative walk that includes a cycle guard.
Changes:
- Converted NSSummary recursive traversals to iterative stack-based traversals with a
visitedset inEntityHandlerandReconUtils.gatherSubPaths. - Added WARN-once logging when repeated references are detected during a walk, surfacing potential NSSummary corruption without crashing Recon.
- Added targeted unit tests to ensure cyclic/self-referential NSSummary graphs terminate and produce de-duplicated results.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/EntityHandler.java | Replaces recursive dir-count and file-size-distribution walks with iterative traversal + visited-set cycle guard and WARN-once logging. |
| hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconUtils.java | Updates gatherSubPaths to an iterative traversal, reducing redundant getNSSummary lookups and preventing stack overflow on cycles. |
| hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/TestReconUtils.java | Adds a timeout-guarded unit test covering gatherSubPaths behavior on a cyclic NSSummary graph. |
| hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/handlers/TestEntityHandlerCycleGuard.java | New unit tests validating EntityHandler walks terminate and de-duplicate correctly on self-loops and multi-node cycles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review follow-up: make logNSSummaryCycle a void method and guard the
warn-once at the call site, so both getTotalFileSizeDist and
getTotalDirCount use the same "else if (!cycleLogged) { ...; cycleLogged
= true; }" idiom that gatherSubPaths already uses. Removes the constant
boolean return and the alreadyLogged parameter. No behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback: the cyclic-tree test only checked subPaths size and de-duplication, which would still pass if the walk wrongly included the traversal root or dropped an expected child. Assert the exact "/volumeId/bucketId/objectId" subpaths (in any order) so the cycle-guard behavior is regression-proof. No production change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Generated-by: Claude Code (Opus 4.8)
What is the problem
https://issues.apache.org/jira/browse/HDDS-15826
Recon serves disk-usage (
/du), file-size-distribution, and directory-count views by walking the NSSummary directory tree. Three code paths do this walk recursively:EntityHandler.getTotalFileSizeDist,EntityHandler.getTotalDirCount, andReconUtils.gatherSubPaths(used when gathering open-file subpaths).Each recurses over
NSSummary.getChildDir()with no protection against a malformed tree. If the persisted NSSummary data is corrupted so that a directory lists itself, or one of its ancestors, as a child, the child set forms a cycle. The recursion then never terminates and Recon dies with aStackOverflowError. On the read path this surfaces as an HTTP 500 from/duand, in the worst case, takes the Recon process down. Because the corruption lives in persisted state, the failure repeats on every request until the underlying data is rebuilt. The recursion also had no upper bound on depth even for a non-cyclic but pathologically deep tree.What this changes
Convert all three walks from recursion to iterative traversal backed by an explicit stack and a
visitedset. Each object is visited, and counted, at most once, so a self-referencing or cyclic child can no longer drive infinite recursion or inflate the result. For a well-formed tree the results are identical to before.Two smaller improvements ride along in the same paths:
gatherSubPathspreviously fetched each node's NSSummary twice (once to null-check a child before descending, once when the child was itself expanded). It now fetches once, on pop, which halves the number ofgetNSSummarylookups on large trees.What this does not change
This is read-side hardening only. It does not attempt to prevent the corruption at write time, and it does not add response paging or a size limit to
/du. Bounding the/duresponse for very large (but well-formed) buckets is a separate concern and is left as a follow-up.How it was tested
TestEntityHandlerCycleGuardcovers a self-referencing directory, a multi-node cycle with a back edge and a self loop, and a clean tree (to confirm unchanged counts) for bothEntityHandlerwalks. Each case runs under a timeout so a regression back to infinite recursion fails fast.TestReconUtilsgains a cyclic-tree case forgatherSubPaths.TestNSSummaryEndpointWithFSO(28 tests) passes unchanged, confirming clean-tree behavior is preserved end to end.checkstyle.shfor the recon module is clean.