Skip to content

Commit 67ff0d2

Browse files
bbrksclaude
andcommitted
CBG-5411: match Couchbase Server Incr behavior for def > int64 max
When def exceeds math.MaxInt64, sync_gateway's gocb-backed Incr casts it to int64 (which goes negative), and gocbcore interprets that as the "do not create if absent" sentinel — the server then returns KEY_ENOENT. Rosmar previously created the doc with the wrapped value instead. Detect the sentinel and return sgbucket.MissingError to match. TestIncr is now table-driven and covers the matrix verified against both backends: amt=0 (read-but-rewrite + CAS bump), uint64-wrap "negative" amt, missing vs existing key, and the new def-sentinel case in both states. Adds a small ptr[T] helper in utils.go used by the test seed values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent db93c1c commit 67ff0d2

3 files changed

Lines changed: 97 additions & 12 deletions

File tree

collection.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,12 @@ func (c *Collection) Incr(_ context.Context, key string, amt, deflt uint64, exp
523523
if err == nil {
524524
result += amt
525525
} else if _, ok := err.(sgbucket.MissingError); ok {
526+
// Couchbase Server's gocb adapter casts def to int64; values > math.MaxInt64
527+
// become negative, which gocbcore interprets as "do not create if absent"
528+
// and the server returns KEY_ENOENT. Match that behavior here.
529+
if int64(deflt) < 0 {
530+
return nil, sgbucket.MissingError{Key: key}
531+
}
526532
result = deflt
527533
} else {
528534
return nil, err

collection_test.go

Lines changed: 87 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"context"
1616
"encoding/json"
1717
"fmt"
18+
"math"
1819
"strconv"
1920
"sync"
2021
"sync/atomic"
@@ -47,25 +48,99 @@ func TestDeleteThenAdd(t *testing.T) {
4748
addToCollection(t, coll, "key", 0, "value")
4849
}
4950

51+
// TestIncr exercises the Incr contract Rosmar shares with Couchbase Server - cross-checked with GoCB/CB Server implementation.
52+
//
53+
// - amt=0 on an existing key still rewrites the doc and bumps CAS (NOT a read-only op).
54+
// - Incr uses uint64 addition with wrap on overflow; "negative" amt
55+
// (e.g. uint64(-1)) decrements via wrap, with no clamp at 0.
56+
// - On a missing key, the delta is not applied — the stored value is def.
57+
// - def must fit in int64. CBS's gocb adapter casts def to int64; values
58+
// > math.MaxInt64 are interpreted as "do not create if absent" and return
59+
// KEY_ENOENT. Rosmar matches this by returning MissingError.
5060
func TestIncr(t *testing.T) {
5161
ctx := t.Context()
5262
ensureNoLeaks(t)
5363
coll := makeTestBucket(t).DefaultDataStore(ctx)
54-
count, err := coll.Incr(ctx, "count1", 1, 100, 0)
55-
assert.NoError(t, err, "Incr")
56-
assert.Equal(t, uint64(100), count)
5764

58-
count, err = coll.Incr(ctx, "count1", 0, 0, 0)
59-
assert.NoError(t, err, "Incr")
60-
assert.Equal(t, uint64(100), count)
65+
// intentional underflows (follows GoCB semantics)
66+
maxU64 := uint64(math.MaxUint64)
67+
negTen := maxU64 - 9 // uint64(-10)
68+
69+
cases := []struct {
70+
desc string // short English description of the scenario
71+
existingValue *uint64 // if non-nil, seed the key via SetRaw before Incr
72+
amt uint64
73+
def uint64
74+
expectErr bool
75+
expectValue uint64
76+
expectPostRaw string
77+
expectCASChanged bool // only checked when the doc existed pre-Incr
78+
}{
79+
{desc: "create zero counter when key missing", amt: 0, def: 0, expectValue: 0, expectPostRaw: "0"},
80+
{desc: "create counter at def when key missing", amt: 0, def: 5, expectValue: 5, expectPostRaw: "5"},
81+
{desc: "amt=0 on existing key returns current value and ignores def", existingValue: ptr(uint64(42)), amt: 0, def: 100, expectValue: 42, expectPostRaw: "42", expectCASChanged: true},
82+
{desc: "missing key returns def, delta is not applied to it", amt: 1, def: 5, expectValue: 5, expectPostRaw: "5"},
83+
{desc: "increment existing counter by amt", existingValue: ptr(uint64(42)), amt: 1, def: 5, expectValue: 43, expectPostRaw: "43", expectCASChanged: true},
84+
{desc: "amt=0 on existing key still rewrites doc and bumps CAS", existingValue: ptr(uint64(42)), amt: 0, def: 0, expectValue: 42, expectPostRaw: "42", expectCASChanged: true},
85+
{desc: "negative amt on missing key stores def (no wrap into def)", amt: maxU64, def: 0, expectValue: 0, expectPostRaw: "0"},
86+
{desc: "negative amt on existing key decrements via uint64 wrap", existingValue: ptr(uint64(42)), amt: maxU64, def: 0, expectValue: 41, expectPostRaw: "41", expectCASChanged: true},
87+
{desc: "amt=-10 on existing key decrements by 10", existingValue: ptr(uint64(100)), amt: negTen, def: 0, expectValue: 90, expectPostRaw: "90", expectCASChanged: true},
88+
{desc: "def > int64 max returns MissingError (matches CBS gocb sentinel)", amt: 1, def: maxU64, expectErr: true},
89+
{desc: "def > int64 max is ignored on existing key (delta still applied)", existingValue: ptr(uint64(42)), amt: 1, def: maxU64, expectValue: 43, expectPostRaw: "43", expectCASChanged: true},
90+
}
6191

62-
count, err = coll.Incr(ctx, "count1", 10, 100, 0)
63-
assert.NoError(t, err, "Incr")
64-
assert.Equal(t, uint64(110), count)
92+
// formatU64 renders a uint64 as "negN" when it represents a wrapped-negative int64,
93+
// so test names read as the caller-intended value (e.g. uint64(-10) → "neg10").
94+
formatU64 := func(v uint64) string {
95+
if int64(v) < 0 {
96+
return fmt.Sprintf("neg%d", -int64(v))
97+
}
98+
return strconv.FormatUint(v, 10)
99+
}
65100

66-
count, err = coll.Incr(ctx, "count1", 0, 0, 0)
67-
assert.NoError(t, err, "Incr")
68-
assert.Equal(t, uint64(110), count)
101+
for _, tc := range cases {
102+
state := "missing"
103+
if tc.existingValue != nil {
104+
state = fmt.Sprintf("existing%d", *tc.existingValue)
105+
}
106+
name := fmt.Sprintf("%s/amt=%s_def=%s_%s", tc.desc, formatU64(tc.amt), formatU64(tc.def), state)
107+
t.Run(name, func(t *testing.T) {
108+
key := name
109+
110+
if tc.existingValue != nil {
111+
raw := []byte(strconv.FormatUint(*tc.existingValue, 10))
112+
require.NoError(t, coll.SetRaw(ctx, key, 0, nil, raw), "setup SetRaw")
113+
}
114+
_, preCAS, preErr := coll.GetRaw(ctx, key)
115+
116+
value, incrErr := coll.Incr(ctx, key, tc.amt, tc.def, 0)
117+
118+
if tc.expectErr {
119+
require.Error(t, incrErr, "expected Incr to error")
120+
require.ErrorAs(t, incrErr, &sgbucket.MissingError{}, "expected MissingError")
121+
if tc.existingValue == nil {
122+
_, _, postErr := coll.GetRaw(ctx, key)
123+
require.Error(t, postErr, "doc should not exist after a failed Incr on missing key")
124+
}
125+
return
126+
}
127+
128+
require.NoError(t, incrErr)
129+
require.Equal(t, tc.expectValue, value, "returned counter value")
130+
131+
postRaw, postCAS, postErr := coll.GetRaw(ctx, key)
132+
require.NoError(t, postErr, "GetRaw after Incr")
133+
require.Equal(t, tc.expectPostRaw, string(postRaw), "post-Incr stored value")
134+
135+
if preErr == nil {
136+
if tc.expectCASChanged {
137+
require.NotEqual(t, preCAS, postCAS, "CAS should have changed")
138+
} else {
139+
require.Equal(t, preCAS, postCAS, "CAS should not have changed")
140+
}
141+
}
142+
})
143+
}
69144
}
70145

71146
// Spawns 1000 goroutines that 'simultaneously' use Incr to increment the same counter by 1.

utils.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,3 +182,7 @@ var (
182182
_ error = &ErrUnimplemented{}
183183
_ error = &DatabaseError{}
184184
)
185+
186+
func ptr[T any](v T) *T {
187+
return &v
188+
}

0 commit comments

Comments
 (0)