Skip to content

Commit 2b970c6

Browse files
authored
Fix valve invariant violation when contention < capacity (#870)
### Background / Why? The valve's gate condition in `lockedEnqueue` used `outstandingCounts[valveID] > 1` to decide whether to valve a new arrival. This is a proxy for "there's already a droppable representative in the CoDel queue", but it's wrong after a grant. When a request is granted, `lockedOnGrant` deletes `droppablePerValve[valveID]` (the droppable became undroppable), but `outstandingCounts` remains elevated because the granted request is still outstanding. Subsequent arrivals for the same valve ID see count > 1 and get valved, but there's no droppable representative to ever trigger promotion. As a result, the invariant ("each nonempty valve always has exactly one droppable entry in the CoDel queue") is broken and requests are stranded indefinitely. This was correct in the Python implementation because: 1. it was a lock, not a semaphore 2. the invariant was slightly different (each nonempty valve has one representative in the codelq, not one DROPPABLE representative). As we noted at the time of coming up with the N-holder invariant, that invariant is actually superior even for the 1-holder (lock) case, it just only mattered in an extreme edge case we likely never saw. The practical impact: with capacity N and M concurrent requests from the same valve ID where M ≤ N, only the first request is ever granted. The rest are serialized one-at-a-time through release→promote→grant chains, leaving N-1 capacity slots unused. This case probably wouldn't ever happen in production since it requires all requests to be for only one valve ID. The fix replaces the `outstandingCounts > 1` check with a direct `droppablePerValve[valveID]` existence check, which directly encodes the invariant. ### Testing new unit tests AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :)
1 parent f9750a5 commit 2b970c6

2 files changed

Lines changed: 80 additions & 1 deletion

File tree

go/vt/vttablet/tabletserver/loadshed/selfcontentionaware_codelq.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ func (q *SelfContentionAwareCoDelQueue) lockedEnqueue(valveID string, priority f
142142

143143
if valveID != "" {
144144
q.outstandingCounts[valveID]++
145-
if q.outstandingCounts[valveID] > 1 {
145+
if q.droppablePerValve[valveID] != nil {
146146
q.pendingRequests[valveID] = append(q.pendingRequests[valveID], req)
147147
return req
148148
}

go/vt/vttablet/tabletserver/loadshed/snake_nholder_test.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,85 @@ func TestSnake_NHolder_ReleaseCallbacks(t *testing.T) {
398398
assert.Equal(t, int64(3), count.Load(), "release callback should fire for each holder")
399399
}
400400

401+
// --- N-holder: valve invariant under sufficient capacity ---
402+
403+
func TestSnake_NHolder_ValveInvariant_AllGranted(t *testing.T) {
404+
const capacity = 10
405+
const M = 5
406+
407+
cfg := defaultSnakeConfig()
408+
cfg.Capacity = func() int { return capacity }
409+
s := NewSnake(cfg)
410+
411+
unlocks := make([]*SafeUnlock, M)
412+
for i := range M {
413+
ctx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond)
414+
u, err := s.Acquire(ctx, "foo")
415+
cancel()
416+
require.NoError(t, err, "request %d should be granted (capacity=%d, holders=%d)", i, capacity, i)
417+
unlocks[i] = u
418+
}
419+
420+
assert.Equal(t, M, s.nGranted(), "all %d requests should be granted concurrently", M)
421+
422+
for _, u := range unlocks {
423+
u.Release()
424+
}
425+
assert.Equal(t, 0, s.nGranted())
426+
}
427+
428+
// --- N-holder: valve invariant under exhausted capacity ---
429+
430+
func TestSnake_NHolder_ValveInvariant_CapacityExhausted(t *testing.T) {
431+
const capacity = 3
432+
433+
cfg := defaultSnakeConfig()
434+
cfg.Capacity = func() int { return capacity }
435+
s := NewSnake(cfg)
436+
437+
unlocks := make([]*SafeUnlock, capacity)
438+
for i := range capacity {
439+
ctx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond)
440+
u, err := s.Acquire(ctx, "foo")
441+
cancel()
442+
require.NoError(t, err, "request %d should be granted", i)
443+
unlocks[i] = u
444+
}
445+
assert.Equal(t, capacity, s.nGranted())
446+
447+
blocked := make(chan struct{})
448+
granted := make(chan *SafeUnlock, 1)
449+
go func() {
450+
close(blocked)
451+
u, err := s.Acquire(context.Background(), "foo")
452+
if err == nil {
453+
granted <- u
454+
}
455+
}()
456+
<-blocked
457+
time.Sleep(10 * time.Millisecond)
458+
459+
s.mu.Lock()
460+
droppable, hasDroppable := s.q.droppablePerValve["foo"]
461+
s.mu.Unlock()
462+
assert.True(t, hasDroppable, "nonempty valve must have a droppable representative")
463+
assert.NotNil(t, droppable)
464+
465+
unlocks[0].Release()
466+
467+
select {
468+
case u := <-granted:
469+
assert.Equal(t, capacity, s.nGranted())
470+
u.Release()
471+
case <-time.After(2 * time.Second):
472+
t.Fatal("blocked request should have been granted after release")
473+
}
474+
475+
for i := 1; i < capacity; i++ {
476+
unlocks[i].Release()
477+
}
478+
}
479+
401480
// --- N-holder: memory cleanup with multi-slot ---
402481

403482
func TestSnake_NHolder_MemoryCleanup(t *testing.T) {

0 commit comments

Comments
 (0)