Fix SHOW_MEMORY crashes on valid input
CI / test (arm64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 22, https://minio.weaselab.dev/public/aarch64/msan-toolchain-22.1.8.tar.zst, debug-arm64, ubuntu-latest-arm64) (pull_request) Successful in 4m8s
CI / release (arm64, , ubuntu-latest-arm64) (pull_request) Successful in 4m9s
CI / pre-commit (pull_request) Successful in 2m9s
CI / test (amd64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 21, https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst, debug, ubuntu-latest-amd64) (pull_request) Successful in 3m56s
CI / test (amd64, -DCMAKE_CXX_FLAGS=-DUSE_64_BIT=1, 21, , 64-bit-versions, ubuntu-latest-amd64) (pull_request) Successful in 3m29s
CI / test (amd64, -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++, 21, , gcc, ubuntu-latest-amd64) (pull_request) Successful in 3m21s
CI / test (amd64, -DUSE_SIMD_FALLBACK=ON, 21, , simd-fallback, ubuntu-latest-amd64) (pull_request) Successful in 3m24s
CI / release (amd64, -DMSAN_TOOLCHAIN_PATH=/opt/msan, ubuntu-latest-amd64) (pull_request) Successful in 5m48s
CI / coverage (pull_request) Failing after 3m26s

`Node::getCapacity()` asserted `!releaseDeferred`, but releasing a deferred
node requires its capacity, so any node replacement aborted an assertion-
enabled `SHOW_MEMORY` build. `partialKeyCapacity` is not repurposed by
`deferRelease`, so there is no reason to reject deferred nodes. Drop the
assertion.

`eraseTree` released a node before visiting its children, so the `parent`
pointers walked by `removeKey` pointed to freed memory (and `deferRelease`
repurposes `parent` as a list link). Carry the search path length of each node
in the worklist, computed while its parent is still alive, and account for it
with the new `removeKey` overload.

Add a test covering the SHOW_MEMORY instrumentation, which the rest of the
test suite doesn't compile with.
This commit is contained in:
weaselbot committed 2026-10-26 18:32:03 -04:00
1 parent ba781b8875
commit bba328f79f
4 files changed
+161 -9

No files matched your search

+89
View File
@@ -0,0 +1,89 @@
// Exercises the SHOW_MEMORY instrumentation, which the other tests don't
// compile with (this target passes -DSHOW_MEMORY=1). The instrumentation has
// its own consistency checks, which abort on failure, and asserts are enabled
// by TEST_FLAGS. Only one ConflictSet may exist at a time, since the
// accounting is process-wide.
#include "ConflictSet.h"
#include "Internal.h"
#include <cassert>
#include <cstdint>
#include <string>
using namespace weaselab;
namespace {
void writePoint(ConflictSet &cs, const std::string &key, int64_t writeVersion) {
ConflictSet::WriteRange w{{(const uint8_t *)key.data(), int(key.size())},
{nullptr, 0}};
cs.addWrites(&w, 1, writeVersion);
}
ConflictSet::Result checkPoint(const ConflictSet &cs, const std::string &key,
int64_t readVersion) {
ConflictSet::ReadRange r{{(const uint8_t *)key.data(), int(key.size())},
{nullptr, 0},
readVersion};
ConflictSet::Result result;
cs.check(&r, &result, 1);
return result;
}
// Everything a ConflictSet allocated must have been accounted for and released
// by the time it's destroyed.
void checkFullyReleased() {
assert(nodeBytes == 0);
assert(partialCapacityBytes == 0);
assert(totalKeys == 0);
assert(keyBytes == 0);
assert(mallocBytes == 0);
}
} // namespace
int main() {
// Splitting a node replaces it, which defers the release of the old node.
// Releasing a deferred node must account for its capacity, and the key it
// held must be accounted for without walking `parent` pointers of nodes that
// have already been released.
{
ConflictSet cs{0};
writePoint(cs, "a", 1);
writePoint(cs, "ab", 2);
assert(checkPoint(cs, "a", 0) == ConflictSet::Conflict);
assert(checkPoint(cs, "ab", 0) == ConflictSet::Conflict);
assert(checkPoint(cs, "b", 0) == ConflictSet::Commit);
assert(nodeBytes > 0);
assert(keyBytes > 0);
}
checkFullyReleased();
// Exceeding the version window erases the tree mid-workload.
{
ConflictSet cs{0};
writePoint(cs, "abc", 1);
const int64_t version = int64_t(1) << 40;
writePoint(cs, "abc", version);
assert(checkPoint(cs, "abc", 0) == ConflictSet::TooOld);
assert(checkPoint(cs, "abc", version) == ConflictSet::Commit);
}
checkFullyReleased();
// Erase a tree with many nodes, both mid-workload and at destruction.
{
ConflictSet cs{0};
for (int i = 0; i < 1000; ++i) {
writePoint(cs, "prefix/" + std::to_string(i), i + 1);
}
const int64_t version = int64_t(1) << 40;
writePoint(cs, "prefix/1", version);
assert(checkPoint(cs, "prefix/1", version) == ConflictSet::Commit);
for (int i = 0; i < 1000; ++i) {
writePoint(cs, "other/" + std::to_string(i), version + i + 1);
}
assert(checkPoint(cs, "other/999", version) == ConflictSet::Conflict);
}
checkFullyReleased();
}