Metabase fixes for tombstone handling#3741
Merged
roman-khimov merged 14 commits intomasterfrom Dec 18, 2025
Merged
Conversation
Delete() method is unused since b57a23d and other methods don't require any wrapping. Signed-off-by: Roman Khimov <roman@nspcc.ru>
Nothing uses it now. Signed-off-by: Roman Khimov <roman@nspcc.ru>
087b459 to
67dc536
Compare
Member
Author
|
Still works at this stage, see https://rest.fs.neo.org/Aku38wpn2xEqfEPxmwyAa7N3VnAkMV8PEVkpLqwKM1z3/4526-1765997752/index.html and https://rest.fs.neo.org/Aku38wpn2xEqfEPxmwyAa7N3VnAkMV8PEVkpLqwKM1z3/4526-1765993701/index.html. All things there are either known (#3736, #3673, #3739) or not related (new nspcc-dev/neofs-contract#565). |
67dc536 to
3dc3177
Compare
This can break expirations/deletions, just about anything. Usually it's not a problem because children are only deleted with parents, but if some forced deletions are done this can be a problem. It's always safer to not delete anyway. Signed-off-by: Roman Khimov <roman@nspcc.ru>
For the scenario to happen properly we need LINK object to be present in metabase, but missing from real FSTree. Only this way we get a LINK in SplitInfo, but then fail to fetch it. Tombstones or garbage marks leave physical LINK in place and all of its metadata as well, while dropping with force removes both physical object and its meta, so the scenario becomes the same as in TestSplitObjectExpirationWithoutLink. Fixes #3669. Signed-off-by: Roman Khimov <roman@nspcc.ru>
Signed-off-by: Roman Khimov <roman@nspcc.ru>
ErrParentObject should be a nice error and shouldn't panic like this:
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0xa1877e]
goroutine 2534 [running]:
testing.tRunner.func1.2({0xbf1ca0, 0x1453eb0})
/usr/lib64/go/1.25/src/testing/testing.go:1872 +0x237
testing.tRunner.func1()
/usr/lib64/go/1.25/src/testing/testing.go:1875 +0x35b
panic({0xbf1ca0?, 0x1453eb0?})
/usr/lib64/go/1.25/src/runtime/panic.go:783 +0x132
github.com/nspcc-dev/neofs-node/internal/errors.(*ParentObjectError).Error(0xebf260?)
<autogenerated>:1 +0x1e
github.com/stretchr/testify/assert.ErrorIs({0x7f05962a5620, 0xc0003f16c0}, {0xebf260, 0xc0000f03c0}, {0xebedc0, 0xc000155030}, {0x0, 0x0, 0x0})
/home/rik/go/pkg/mod/github.com/stretchr/testify@v1.11.1/assert/assertions.go:2182 +0xf6
github.com/stretchr/testify/require.ErrorIs({0xec3408, 0xc0003f16c0}, {0xebf260, 0xc0000f03c0}, {0xebedc0, 0xc000155030}, {0x0, 0x0, 0x0})
/home/rik/go/pkg/mod/github.com/stretchr/testify@v1.11.1/require/require.go:372 +0xe5
github.com/nspcc-dev/neofs-node/pkg/local_object_storage/metabase_test.assertECPartsError(0xc0003f16c0, {0xebf260, 0xc0000f03c0}, {0xc0001ad200, 0xa, 0x10})
/home/rik/dev/neofs-node/pkg/local_object_storage/metabase/ec_test.go:717 +0x7a
github.com/nspcc-dev/neofs-node/pkg/local_object_storage/metabase_test.testExistsEC(0xc0003f16c0)
/home/rik/dev/neofs-node/pkg/local_object_storage/metabase/ec_test.go:486 +0x8d3
testing.tRunner(0xc0003f16c0, 0xde6758)
/usr/lib64/go/1.25/src/testing/testing.go:1934 +0xea
created by testing.(*T).Run in goroutine 2491
/usr/lib64/go/1.25/src/testing/testing.go:1997 +0x465
Signed-off-by: Roman Khimov <roman@nspcc.ru>
root is checked 20 lines above, it's always true here. Signed-off-by: Roman Khimov <roman@nspcc.ru>
In case LINK is missing we're using search to find objects that need to be deleted, but this can work incorrectly for expired children since search never returns outdated objects. I believe this fixes #3715, that's the scenario (and log) that can be seen in UTs and it leads to the last object part being stuck in the GC loop: parent object should be deleted, but searching doesn't return an outdated object. The new API is also somewhat more efficient. Signed-off-by: Roman Khimov <roman@nspcc.ru>
Parent locked -> child locked. Parent deleted -> child deleted. Parent expired -> child expired. This makes sense overall and this is especially important for future updates to tombstone handling, we need some way to derive child status without graveyard marks for all children, just using the meta index that does have this data anyway (since TS are broadcasted) Signed-off-by: Roman Khimov <roman@nspcc.ru>
Pre-2.18 tombstones are not supported after 45150c9, so this code is no longer relevant. Signed-off-by: Roman Khimov <roman@nspcc.ru>
It's never false in production code now (the last user was meta resync code) and tests don't care much. Signed-off-by: Roman Khimov <roman@nspcc.ru>
All production code uses this call for forced removals. This leaves Engine.inhume for tombstones only. Signed-off-by: Roman Khimov <roman@nspcc.ru>
It's FUBAR. We better hide it till #3740 makes something more meaningful out of it. Signed-off-by: Roman Khimov <roman@nspcc.ru>
The way it's intended to work is to collect all related objects, like it does for EC. But for regular splits in only catched last/first/link, completely missing middle children. Signed-off-by: Roman Khimov <roman@nspcc.ru>
3dc3177 to
78dd4af
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3741 +/- ##
==========================================
+ Coverage 26.60% 26.66% +0.06%
==========================================
Files 656 656
Lines 41552 41611 +59
==========================================
+ Hits 11053 11095 +42
- Misses 29482 29503 +21
+ Partials 1017 1013 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
carpawell
approved these changes
Dec 18, 2025
|
|
||
| _, err = e.Get(parentAddr) | ||
| require.ErrorAs(t, err, &splitErr) | ||
| require.Equal(t, linkObj.GetID(), splitErr.SplitInfo().GetLink()) // Pretends to be here, but not in fact. |
Member
There was a problem hiding this comment.
tbh, do not get what this test tests, but it tests smth for sure
Member
Author
There was a problem hiding this comment.
Follow the log line it checks for, it's a very specific and abnormal case, but still it can happen.
| fs.AddFirstSplitObjectFilter(objectSDK.MatchStringEqual, firstID) | ||
| tmpAddr.SetObject(firstID) | ||
| children = append(children, tmpAddr) | ||
| res, err := e.collectRawWithAttribute(addr.Container(), objectSDK.FilterFirstSplitObject, firstID[:]) |
Member
There was a problem hiding this comment.
calling addr.Container() duplicated too many times for me
| res = append(res, oid.NewAddress(addr.Container(), firstID)) | ||
| return res | ||
| } | ||
| e.log.Warn("failed to collect objects with first ID", zap.Stringer("addrBeingInhumed", addr), zap.Error(err)) |
Member
There was a problem hiding this comment.
firstID can be added to log too
Member
Author
There was a problem hiding this comment.
Object address should be sufficient to get other things from it.
| if err == nil { | ||
| return res | ||
| } | ||
| e.log.Warn("failed to collect objects with split ID", zap.Stringer("addrBeingInhumed", addr), zap.Error(err)) |
|
|
||
| var haveUnhandledID = func() bool { | ||
| for i := range ids { | ||
| if idx[i] < len(ids[i]) { |
Member
There was a problem hiding this comment.
idx and ids are too similar var names for me, was confused a couple of times
| // OK, we're not comparing containers, but it's all | ||
| // the same and current SDK doesn't make it easy to | ||
| // make any comparisons. | ||
| return bytes.Compare(a.Object().Marshal(), b.Object().Marshal()) |
Member
There was a problem hiding this comment.
even Marshal? dont mind in fact
Member
Author
There was a problem hiding this comment.
I have a lot of questions for SDK. It's absolutely ridiculous how hard it is to do some simple things.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return res, err |
Member
There was a problem hiding this comment.
return res, nil? or even always return res, err without if
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
2 participants
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.
A part of #3476.