Fix BITFIELD signed 64-bit overflow detection for OVERFLOW SAT/FAIL

[hexonal]

Summary

BITFIELD ... OVERFLOW SAT|FAIL INCRBY i64 ... silently fails to detect overflow. SAT wraps instead of saturating; FAIL succeeds (and writes) instead of rejecting the write and returning nil.

Root cause

In CheckSignedBitfieldOverflow (libs/server/Resp/Bitmap/BitmapManagerBitfield.cs), the overflow check is:

var overflow = (ulong)(result & ~mask) > 0 && value >= 0 && incrBy > 0;

At bitCount == 64, mask is set to -1 (all bits set), so ~mask == 0, making result & ~mask always 0 — the condition can never be true, regardless of whether the addition actually overflowed. WRAP isn't affected by this, since its output value already comes from plain two's-complement long addition (result = value + incrBy) independent of the overflow flag.

Fix

Special-case bitCount == 64, detecting overflow via the sign flip on the raw result instead — mirroring the existing bitCount == 64 special case for maxVal a few lines below in the same function's SAT branch:

var overflow = bitCount == 64
    ? result < 0 && value >= 0 && incrBy > 0
    : (ulong)(result & ~mask) > 0 && value >= 0 && incrBy > 0;

Tests

test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs has two of its own duplicated copies of overflow-checking logic (CheckSignedBitfieldOverflow and CheckSignedBitfieldOverflowRedis) used as differential-testing oracles by several existing tests — they had the identical class of bug at bitCount == 64 and are updated to match, otherwise several existing tests (e.g. BitmapBitfieldGrowingTest) would now fail by disagreeing with the corrected production code.

Added BitmapBitfieldSigned64OverflowTest, covering SAT/FAIL at bitCount == 64 in both the overflow and underflow directions. Confirmed it fails against the pre-fix code (reproduces the silent-wrap/silent-success bug) and passes with the fix.

Note: while testing FAIL, I noticed the stored value is unconditionally overwritten even when FAIL rejects the operation (rather than left untouched, as OVERFLOW FAIL's "no operation is performed" semantics would suggest) — this appears to be a separate, pre-existing issue independent of bitCount, not something introduced or fixed here, so I left it out of scope and out of the new test's assertions.

* Fix `BITFIELD ... OVERFLOW SAT|FAIL` silently failing to detect overflow on signed 64-bit (`i64`) fields.

[Copilot]

✅ Ready to approve

The change is narrowly scoped, fixes a clearly identified logic gap for i64 at 64 bits, and includes targeted regression coverage plus aligned test oracles.

Note: this review does not count toward required approvals for merging.

Pull request overview

Fixes incorrect overflow handling for BITFIELD ... OVERFLOW SAT|FAIL INCRBY i64 ... when the field width is 64 bits (full signed long), where the prior “extra high bits” check could never trigger and caused SAT to wrap and FAIL to incorrectly succeed.

Changes:

• Special-cases bitCount == 64 signed overflow detection using sign-flip behavior of two’s-complement addition.
• Updates the test-side differential “oracle” overflow helpers to match the corrected production semantics at 64 bits.
• Adds a focused regression test covering both overflow and underflow for i64 with OVERFLOW SAT and OVERFLOW FAIL.

File summaries

File	Description
libs/server/Resp/Bitmap/BitmapManagerBitfield.cs	Corrects signed 64-bit overflow detection for BITFIELD OVERFLOW SAT/FAIL by handling bitCount == 64 explicitly.
test/standalone/Garnet.test.complexstring/GarnetBitmapTests.cs	Updates test overflow oracles for the 64-bit case and adds a regression test for SAT/FAIL overflow/underflow behavior on i64.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.