Skip to content

fix: write-buffer-size overflow #3193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Z-G-H1 wants to merge 11 commits into
base: unstable
Choose a base branch
from
Open

fix: write-buffer-size overflow #3193

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3cb91ee
修复v3.5.5版本中write-buffer-size溢出问题
Nov 5, 2025
a587e93
add test for fix #3183
Z-G-H1 Nov 24, 2025
9402fc9
add test case
Z-G-H1 Nov 25, 2025
0c82d04
Merge branch 'OpenAtomFoundation:unstable' into fix-for-unstable
Z-G-H1 Dec 5, 2025
2352b94
modify ci
Z-G-H1 Dec 9, 2025
93150ed
Upgrade macOS version in GitHub Actions workflow
Z-G-H1 Dec 9, 2025
e9be2da
Upgrade GCC version in CI workflow
Z-G-H1 Dec 9, 2025
a79a94a
fix hashes_test
Z-G-H1 Dec 9, 2025
933d31e
fix test
Z-G-H1 Dec 9, 2025
bf44a12
fix hashes_test
Z-G-H1 Dec 10, 2025
658b9d5
fix test
Z-G-H1 Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 11 additions & 22 deletions .github/workflows/pika.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,24 +170,12 @@ jobs:

- name: Extreme Disk Cleanup
run: |

rm -rf /usr/local/share/* || true
rm -rf /usr/share/doc/* || true
rm -rf /usr/share/man/* || true
rm -rf /var/cache/* || true

find ${{ github.workspace }} -name "*.o" -type f -delete || true
find ${{ github.workspace }} -name "*.a" -type f -delete || true
find ${{ github.workspace }} -name "*.la" -type f -delete || true
find ${{ github.workspace }} -name "*.so" -type f -delete || true
find ${{ github.workspace }} -name "*.pyc" -type f -delete || true

rm -rf ${{ github.workspace }}/.git || true

rm -rf /__w/pikiwidb/pikiwidb/buildtrees 2>/dev/null || true
rm -rf /__w/pikiwidb/pikiwidb/deps 2>/dev/null || true
find /__w/pikiwidb/pikiwidb -type f \( -name "librocksdb.a" -o -name "libprotoc.a" -o -name "libprotobuf.a" \) -delete 2>/dev/null || true
find /__w/pikiwidb/pikiwidb -type f \( -name "*.o" -o -name "*.a" -o -name "*.la" -o -name "*.so" -o -name "*_test" \) ! -path "*/build/pika" -delete 2>/dev/null || true
rm -rf /__w/pikiwidb/pikiwidb/.git 2>/dev/null || true
Comment on lines +173 to +177
Copy link

@coderabbitai coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the find pattern correctly excludes build/pika artifacts

# Create test directory structure
TEST_DIR=$(mktemp -d)
mkdir -p "$TEST_DIR/build/pika/bin"
mkdir -p "$TEST_DIR/other/test"

touch "$TEST_DIR/build/pika/bin/pika"
touch "$TEST_DIR/build/pika/lib.a"
touch "$TEST_DIR/other/test/lib.a"

echo "=== Files that WILL be deleted (should NOT include build/pika) ==="
find "$TEST_DIR" -type f \( -name "*.o" -o -name "*.a" \) ! -path "*/build/pika" -print

echo ""
echo "=== Files under build/pika (should be preserved) ==="
find "$TEST_DIR/build/pika" -type f

# Additional test: verify with recursive paths
echo ""
echo "=== Testing with nested paths under build/pika ==="
mkdir -p "$TEST_DIR/build/pika/subdir"
touch "$TEST_DIR/build/pika/subdir/lib.a"

echo "With nested path:"
find "$TEST_DIR" -type f \( -name "*.o" -o -name "*.a" \) ! -path "*/build/pika*" -print

echo ""
echo "Original pattern (without asterisk after pika):"
find "$TEST_DIR" -type f \( -name "*.o" -o -name "*.a" \) ! -path "*/build/pika" -print

# Cleanup
rm -rf "$TEST_DIR"

Repository: OpenAtomFoundation/pikiwidb

Length of output: 198

🏁 Script executed:

#!/bin/bash
# Test find pattern behavior - NO repo access needed

# Create test directory structure
TEST_DIR=$(mktemp -d)
mkdir -p "$TEST_DIR/build/pika/bin"
mkdir -p "$TEST_DIR/other/test"

touch "$TEST_DIR/build/pika/bin/pika"
touch "$TEST_DIR/build/pika/lib.a"
touch "$TEST_DIR/other/test/lib.a"
touch "$TEST_DIR/test.a"

echo "=== Directory structure ==="
find "$TEST_DIR" -type f

echo ""
echo "=== Testing pattern: ! -path '*/build/pika' ==="
echo "Files WILL be deleted with this pattern:"
find "$TEST_DIR" -type f \( -name "*.a" \) ! -path "*/build/pika" -print

echo ""
echo "=== Testing pattern: ! -path '*/build/pika*' (with asterisk) ==="
echo "Files WILL be deleted with improved pattern:"
find "$TEST_DIR" -type f \( -name "*.a" \) ! -path "*/build/pika*" -print

echo ""
echo "=== Testing pattern: ! -path '*/build/pika/*' (with trailing slash) ==="
echo "Files WILL be deleted with slash pattern:"
find "$TEST_DIR" -type f \( -name "*.a" \) ! -path "*/build/pika/*" -print

# Cleanup
rm -rf "$TEST_DIR"

Repository: OpenAtomFoundation/pikiwidb

Length of output: 198

🌐 Web query:

find -path glob pattern matching behavior exclude directory

💡 Result:

Short summary

  • find -path matches the whole file path (relative to the start point) using shell globs; '/' and '.' are not special to the pattern. [1]
  • To exclude a directory and its contents efficiently, use -prune together with a -path that matches the directory exactly (including the same prefix as the find start point), then use -o for the rest of the expression. The ordering matters: prune must be evaluated before tests like -type. [1][2]

Typical examples

  • Exclude a directory named dir_to_exclude under the current dir:
    find . -path "./dir_to_exclude" -prune -o -print
    (prints everything except dir_to_exclude and its contents). [2]

  • Exclude by pattern (all files under any dir named node_modules):
    find . -path "/node_modules/" -prune -o -name "*.js" -print
    (prunes those trees, then finds .js elsewhere). [2][3]

Notes and pitfalls

  • Make sure your -path pattern matches exactly how find would print the path (e.g., include ./ when starting at . or include the leading directory when starting elsewhere). Otherwise prune may not match. [2][3]
  • Using -not -path only prevents printing matches but still descends into excluded trees; use -prune to avoid recursing into them. [2][3]

Sources: man page for find ( -path / -prune ) and common examples/discussion. [1][2][3]

References
[1] find man page (–path description).
[2] Stack Overflow — "How do I exclude a directory when using find?" (examples using -prune).
[3] Stack Overflow — examples and explanation of -path matching and -prune.

Fix find pattern to correctly preserve build/pika artifacts.

The pattern ! -path "*/build/pika" will not exclude files under the build/pika directory. A file like build/pika/lib.a has the full path build/pika/lib.a, which does not match the glob pattern */build/pika (the pattern only matches the directory name itself, not paths within it). Use ! -path "*/build/pika/*" instead. Better yet, use -prune for both correctness and efficiency: find ... -path "*/build/pika" -prune -o \( -name "*.o" -o -name "*.a" ... \) -delete.

🤖 Prompt for AI Agents 
.github/workflows/pika.yml lines 173-177: the find command's exclusion `! -path
"*/build/pika"` doesn't exclude files under build/pika (it only matches the
directory itself); update the command to either change the exclusion to `! -path
"*/build/pika/*"` or, preferably, use find's pruning for correctness and
efficiency — i.e. add `-path "*/build/pika" -prune -o` before the name tests so
files under that directory are skipped, then keep the name predicates and
`-delete`.

df -h

echo "Largest directories:"
du -h --max-depth=2 / 2>/dev/null | sort -hr | head -20

- name: Create Log Directories
run: |
Expand Down Expand Up @@ -225,7 +213,7 @@ jobs:

build_on_macos:

runs-on: macos-13
runs-on: macos-14

steps:
- uses: actions/checkout@v4
Expand All @@ -238,17 +226,18 @@ jobs:
- name: ccache
uses: hendrikmuhs/[email protected]
with:
key: macos-13
key: macos-14

- name: Install Deps
run: |
brew list --versions cmake && brew uninstall --ignore-dependencies --force cmake || true
brew install gcc@10 automake cmake make binutils
brew install gcc@13 automake cmake make binutils

- name: Configure CMake
run: |
export CC=/usr/local/opt/gcc@10/bin/gcc-10
cmake -B build -DCMAKE_C_COMPILER=/usr/local/opt/gcc@10/bin/gcc-10 -DUSE_PIKA_TOOLS=ON -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache
GCC_PREFIX=$(brew --prefix gcc@13)
export CC=$GCC_PREFIX/bin/gcc-13
cmake -B build -DCMAKE_C_COMPILER=$GCC_PREFIX/bin/gcc-13 -DUSE_PIKA_TOOLS=ON -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache

- name: Build
run: |
Expand Down
2 changes: 1 addition & 1 deletion include/pika_conf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ class PikaConf : public pstd::BaseConf {
TryPushDiffCommands("max-background-jobs", std::to_string(value));
max_background_jobs_ = value;
}
void SetWriteBufferSize(const int& value) {
void SetWriteBufferSize(int64_t value) {
std::lock_guard l(rwlock_);
TryPushDiffCommands("write-buffer-size", std::to_string(value));
write_buffer_size_ = value;
Expand Down
2 changes: 1 addition & 1 deletion src/pika_admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2794,7 +2794,7 @@ void ConfigCmd::ConfigSet(std::shared_ptr<DB> db) {
res_.AppendStringRaw("-ERR Set write-buffer-size wrong: " + s.ToString() + "\r\n");
return;
}
g_pika_conf->SetWriteBufferSize(static_cast<int>(ival));
g_pika_conf->SetWriteBufferSize(ival);
res_.AppendStringRaw("+OK\r\n");
} else if (set_item == "max-write-buffer-num") {
if (pstd::string2int(value.data(), value.size(), &ival) == 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/storage/tests/hashes_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ TEST_F(HashesTest, HIncrbyfloat) {
// operation is performed
s = db.HIncrbyfloat("HINCRBYFLOAT_KEY", "HINCRBYFLOAT_FIELD", "12.3456", &new_value);
ASSERT_TRUE(s.ok());
ASSERT_EQ(new_value, "12.3456");
ASSERT_NEAR(std::stod(new_value.ToString()), 12.3456, 1e-9);
Copy link

@coderabbitai coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unrelated change: scope creep in bug-fix PR.

This floating-point comparison improvement is unrelated to the write-buffer-size overflow fix described in the PR objectives. While using ASSERT_NEAR for floating-point comparisons is technically correct and preferred over string equality, this change should be in a separate PR focused on test improvements.

Additionally, line 391 contains a commented-out similar assertion (//ASSERT_EQ(new_value, "12.3456");), suggesting this fix is incomplete. If you're addressing floating-point comparison issues in tests, all instances should be handled consistently.

Recommendation: Consider moving this test improvement to a separate PR and ensure all floating-point comparisons in the test are handled consistently (including the commented-out assertion at line 391).

🧰 Tools
🪛 Cppcheck (2.18.0)

[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.

(normalCheckLevelMaxBranches)

🤖 Prompt for AI Agents 
In src/storage/tests/hashes_test.cc around line 388, the test change replacing a
string equality with ASSERT_NEAR is unrelated to the write-buffer-size overflow
fix and introduces scope creep; revert this change in this PR (restore the
original assertion) and move the floating-point comparison improvement to a
separate test-improvement PR where you should consistently update all similar
checks (including the commented-out ASSERT_EQ at ~line 391) to use an
appropriate floating-point assertion such as ASSERT_NEAR with a defined epsilon.

s = db.HGet("HINCRBYFLOAT_KEY", "HINCRBYFLOAT_FIELD", &new_value);
ASSERT_TRUE(s.ok());
//ASSERT_EQ(new_value, "12.3456");
Expand Down
12 changes: 12 additions & 0 deletions tests/integration/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,18 @@ var _ = Describe("Server", func() {
Expect(r.Val()).To(Equal("OK"))
})

It("should ConfigSet write-buffer-size large value", func() {
// Test for fix: when setting write-buffer-size value larger than 2147483647,
// the value should not become negative
configSet := client.ConfigSet(ctx, "write-buffer-size", "3000000000")
Expect(configSet.Err()).NotTo(HaveOccurred())
Expect(configSet.Val()).To(Equal("OK"))

configGet := client.ConfigGet(ctx, "write-buffer-size")
Expect(configGet.Err()).NotTo(HaveOccurred())
Expect(configGet.Val()).To(Equal(map[string]string{"write-buffer-size": "3000000000"}))
})

It("should ConfigSet maxmemory", func() {
configGet := client.ConfigGet(ctx, "maxmemory")
Expect(configGet.Err()).NotTo(HaveOccurred())
Expand Down
Loading