fix(subtreevalidation): nil-check subtree before deleting orphans#771
Open
ordishs wants to merge 2 commits intobsv-blockchain:mainfrom
Open
fix(subtreevalidation): nil-check subtree before deleting orphans#771ordishs wants to merge 2 commits intobsv-blockchain:mainfrom
ordishs wants to merge 2 commits intobsv-blockchain:mainfrom
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Summary: The fix correctly addresses issue #4570 by adding nil-checks before dereferencing Findings: [Minor] Missing documentation for GetSubtreeExists Positive observations:
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-04-28 19:07 UTC |
…lock_subtrees (refs #4570)
|
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
1 participant
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.
Closes #4570.
Summary
subtreesHandlercallsValidateSubtreeInternal, which legitimately returns(nil, nil)when the subtree already exists in the store (a race betweenTryLockIfFileNotExistsand another goroutine completing the write). The handler then panicked onsubtree.Nodes. Severity HIGH because any concurrent subtree-write race can crash the goroutine.Fix
Insert a
if subtree == nil { return nil }between the error check and the orphan-deletion loop. Matches the audit's prescribed shape and mirrors the existing guard inServer.CheckSubtree(line 802).This PR also implements the previously stubbed
GetSubtreeExistsso it actually consultssubtreeStore.Existsfor the subtree file. Without this, the(nil, nil)race path documented in the audit was unreachable - so the regression test could not exercise the bug. Now the existence check works as the comments described and the new defensive guard is properly covered.Test plan
TestSubtreesHandler_NilSubtree) drives the handler down the(nil, nil)return path; asserts no panic and no error.go test -race ./services/subtreevalidation/...passes (241s).