Translate Quip's Loadshed-Lock to Go#840
Conversation
314f3e5 to
06d5590
Compare
| // SafeUnlock is a handle for releasing a lock. Only the goroutine that | ||
| // acquired the lock should call Release. Release is idempotent. | ||
| SafeUnlock struct { | ||
| l *Lock | ||
| nonce uint64 | ||
| once sync.Once | ||
| err error | ||
| } |
There was a problem hiding this comment.
this pattern appears to not be idiomatic in go. if the usage of that the lock is taken and released in the same function, maybe we don't need the additional proptection and can get away with a simpler abstraction of an "erroring mutex"
err := emu.Lock()
if err != nil {
return ...
}
defer emu.Unlock()There was a problem hiding this comment.
we discussed this verbally today. Assuming we keep the lock(semaphore) holder inside the codelq, the use-case of transactions means we can't do this since it's multiple requests from the client, which implies multipl estackframes.
Fortunately, it's not completely without precedent. Claude claims idiomatic Go for this is store the release handle on the struct that owns the resource lifetime, which is StatefulConnection. It already does exactly this with the DB connection itself:
// stateful_connection.go
type StatefulConnection struct {
dbConn *connpool.PooledConn // acquired in Begin, released in Commit/Rollback
ConnID tx.ConnID
// ...
}
func (sc *StatefulConnection) Release(reason tx.ReleaseReason) {
sc.pool.unregister(sc.ConnID, ...)
sc.dbConn.Recycle() // ← returns conn to pool
sc.dbConn = nil
}|
|
||
| if contentionID != "" { | ||
| s.outstandingCounts[contentionID]++ | ||
| if s.outstandingCounts[contentionID] > 1 { |
There was a problem hiding this comment.
we need to put some thought into whether 1 is the right number here. it made sense when doing single dispatch, but when we do multiple and possibly variable dispatch i don't know offhand what the right answer is.
| IntervalNs func() int64 | ||
| TargetNs func() int64 | ||
| Exponent func() float64 | ||
| MinDropDelayNs func() int64 |
There was a problem hiding this comment.
for Quip we never overrode the default we set, 100ns -- maybe just don't even expose this?
Code Review: Interactive Walkthrough ResultsBrett and I did a function-by-function review comparing this Go translation against the Python original. 23 items identified — 1 bug, 3 architectural, 5 refactoring, 14 naming/clarity. Bug Found & FixedCancel-vs-grant race in Acquire's double-select (commit Race window: releaser sets Architecture (3 items)
Refactoring (5 items)
Naming & Clarity (14 items)
All items will be addressed in a follow-up commit on this branch. |
|
Thanks for the contribution! Before we can merge this, we need @mattlord @vitess-bot @nickvanw @maxenglander @mhamza15 @arthurschreiber to sign the Salesforce Inc. Contributor License Agreement. |
Port the Python loadshed-lock system to Go for production use on VTTablets. The lock uses a CoDel (Controlled Delay) algorithm to detect persistent queue buildup and shed load by dropping low-priority requests, while a self-contention-aware valve system prevents fan-out patterns from artificially triggering drops. Key design decisions for Go's concurrency model: - Single shared mutex across all layers (Lock, SelfAwareCoDelQueue, CoDelQueue) mirrors Python's single-threaded asyncio guarantee - chan error (capacity 1) per request replaces asyncio.Future - Direct method calls within critical sections replace Python's future-callback inter-layer communication - Cancel-vs-grant race handling for Go's non-deterministic select Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
- Inline `DroppedRequestError` into `codelq.go`, remove `errors.go` - Rename `SelfAwareCoDelQueue` → `SelfContentionAwareCoDelQueue` - Rename `cq` field → `codelq` for explicitness - Rename timer functions to `locked*` prefix for consistency (`scheduleDropTimerLocked` → `lockedScheduleDropTimer`, etc.) - Add `PriorityUndroppable` sentinel constant in `request.go` - Replace `context.Background()` with `t.Context()` in all tests - Run `gofumpt` and `goimports` on all files Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Brett Wines <bwines@slack-corp.com>
Resolves golangci-lint failures from the Static Code Checks CI job: - Replace math/rand with math/rand/v2 (depguard rule) - Convert all wg.Add(1)+go func patterns to wg.Go (waitgroup modernize) - Add //nolint:modernize to NewPriority and math.Inf call sites Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> Signed-off-by: Brett Wines <bwines@slack-corp.com>
The CoDel queue insertion logic (set enqueuedAt, PushBack, droppableLen, timer schedule signaling) was duplicated between CoDelQueue.lockedEnqueue and SelfContentionAwareCoDelQueue.codelqEnqueue. Extract it into CoDelQueue.lockedEnqueueRequest so the valve layer delegates instead of reimplementing. Mirrors the Python split (enqueue vs enqueue_request). Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Signed-off-by: Brett Wines <bwines@slack-corp.com>
…ield lockedPeek needed to check whether a signaled request was granted (nil) or dropped (error) without consuming the channel value. It did this by reading from the channel and writing the value back — correct under the mutex but fragile and hard to reason about. Add a signal() method on Request that writes both an inspectable result field and the blocking channel atomically. lockedPeek now reads result directly. The channel is still used for blocking in Lock.Acquire. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Signed-off-by: Brett Wines <bwines@slack-corp.com>
The old TestLock_Stress_SelfContention used defaultLockConfig() which hardcodes ContentionID to return "" — the valve was never exercised. TestLock_Stress_SelfContention_Proper had weak assertions. Replace both with 6 focused stress tests that use a sync.Map + goroutineID() pattern to set per-goroutine contention IDs, ensuring the valve mechanism is actually tested under concurrent load: 1. MutualExclusion: 10 IDs x 10 goroutines, per-ID + global atomics 2. ValveSerializationOrder: FIFO assertion for single contention ID 3. DropPromotionChain: aggressive CoDel, granted+dropped=total per ID 4. CancelInValve: cancel every other waiter, verify promotion chain 5. MixedCancelDropGrant: CoDel+max-age+timeouts, no lost requests 6. HighConcurrency_Sustained: 500ms sustained cycling, mutual exclusion All pass with -race -count=5. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Signed-off-by: Brett Wines <bwines@slack-corp.com>
Lock.runDropTimer reached through two layers (l.sq.codelq) to find droppable requests and run the CoDel state machine, then called back up to l.sq.lockedDropActive for valve promotion. This coupled Lock to CoDelQueue internals (list.Element, Request casting, contentionID). Give SelfContentionAwareCoDelQueue its own lockedRunScheduledDrop that encapsulates the find-and-drop-with-promotion logic. Lock now calls l.sq.lockedRunScheduledDrop() without knowing about CoDelQueue, list elements, or how drops trigger valve promotion — matching the Python original where CoDelQueue owned all drop logic and promotion happened via Future callbacks. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Signed-off-by: Brett Wines <bwines@slack-corp.com>
`scheduleDropIfNeeded` was a no-op that existed as a placeholder from the Python translation. Timer scheduling is signaled via return values from `lockedEnqueue` and `lockedRunScheduledDrop` — the no-op was never called for effect. `stopDropTimer` → `lockedClearTimerFlag` makes the method honest about what it does (clears a bookkeeping flag) and consistent with the `locked*` prefix convention. AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Brett Wines <bwines@slack-corp.com>
When lockedDequeue exits the dropping state (sojourn < target), the drop timer was cleared but never re-armed. If droppable items remain in the queue and no new enqueue arrives, CoDel never fires — a liveness gap. Port the Python behavior: re-arm at the healthy interval. The fix changes lockedDequeue to return (req, needSchedule, delayNs) so callers can propagate the reschedule signal up through SelfContentionAwareCoDelQueue and Lock.release/releaseInternal. Also fixes lockedPromote which was discarding the needSchedule signal from codelqEnqueue — valve promotions now correctly propagate the timer schedule signal to the Lock layer. AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Brett Wines <bwines@slack-corp.com>
The race window: after the releaser sets holder=req and unlocks but BEFORE signaling, the cancelling goroutine's inner non-blocking select takes default (done channel empty), acquires the mutex, and previously would call lockedCancel on itself — the current holder. This leaked the grant (17% reproduction rate in stress test): release callbacks skipped, max-age timer fires on stale holder, l.holder points to a cancelled request. Fix: after acquiring the mutex in the default branch, check if we were granted during the race window (l.holder == req). If so, drain the pending signal from the done channel (the releaser is about to send it) and call releaseInternal to hand the lock to the next waiter. AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Brett Wines <bwines@slack-corp.com>
125651d to
3f8af27
Compare
- Drop timer via callback injection (matches Python's call_later design) - valveID as explicit Acquire parameter instead of config callback - Droppability derived from priority (remove separate boolean) - Rename done→result, enqueuedAt→enqueuedAtNs, contentionID→valveID - Move drop orchestration into SelfContentionAwareCoDelQueue - Fix missing decrementOutstanding on dequeue path - Remove dead code (goroutineID, scheduleDropIfNeeded out-params) - Simplify test helpers to match new callback-injection APIs AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lockedPeek defensively removes done-with-error requests from the CoDel queue head, but was not decrementing droppableLen or notifying the self-contention layer to decrement outstandingCounts. This could cause both counters to drift if the defensive path ever fires. Fix: add onPeekCleanup callback to CoDelQueue (mirroring the scheduleDropTimer injection pattern). CoDelQueue now decrements droppableLen inline and calls the callback for each cleaned-up request. SelfContentionAwareCoDelQueue provides the callback to decrement outstandingCounts. Python doesn't have this issue because asyncio Future callbacks fire automatically on completion, so the equivalent bookkeeping is always triggered regardless of which code path observes the done state. AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI uses Go 1.24.13 (per go.mod) which doesn't have WaitGroup.Go
(added in Go 1.25). Replace all 23 occurrences in lock_stress_test.go
and lock_test.go with the traditional wg.Add(1); go func() { defer
wg.Done(); ... }() pattern.
Also applies gofmt to fix minor alignment drift in request.go and
codelq.go.
AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if len(q.pendingRequests[valveID]) == 0 { | ||
| delete(q.pendingRequests, valveID) | ||
| } | ||
| } |
There was a problem hiding this comment.
valves are FIFO, I'll touch this up in a subsequent PR. I don't think this is O(N) based on the caller pattern, but we can make that a hard guarantee by adjusting the implementation a bit.
|
squash-merged in 4191f37 |
Background / Why?
VTTablets under high load need a mechanism to shed excess requests rather than letting queue depth grow unbounded. The Python loadshed-lock system implements this using the CoDel (Controlled Delay) algorithm — the same algorithm used in Linux's
fq_codelqueueing discipline — to detect persistent queue buildup and drop low-priority requests before they accumulate latency that compounds downstream.This PR ports that system to Go. The core challenge is that Python's asyncio provides implicit single-threaded synchronization, while Go requires explicit synchronization under true parallelism. The design uses a single shared
sync.Mutexacross all layers to mirror Python's single-threaded guarantee, withchan errorreplacingasyncio.Futurefor per-request signaling.Architecture
Request lifecycle events
lockedMarkNotDroppable+signal(nil)lockedDequeuelockedDropActive→signal(err)lockedCancel/lockedRemoveSummary
Requestwith priority (*float64),resultchannel,signal()for atomic write to inspectableoutcomefield + blocking channel,container/listback-pointer for O(1) removal, unexportedpriorityUndroppablesentinel, droppability derived from priority (no separate boolean)DroppedRequestError: healthy/dropping state machine, control law (interval / count^exponent), drop timer scheduling via injected callback, max 100 drops per timer fire, sojourn-time-based state transitionslockedRunScheduledDropwhich encapsulates drop-finding + valve promotionLock(public API) +SafeUnlockwith nonce verification,Acquire(ctx, valveID)with explicit valve ID parameter, max-age forced release, cancel-vs-grant race handling, release callbacks with panic recoveryKey design decisions
scheduleDropTimer(delayNs)internally whenever it needs a timer (matching Python'scall_later). Lock provides the callback. This eliminates the error-prone out-parameter pattern that caused missed reschedules.Acquire(ctx, valveID string)takes the valve ID directly rather than via a config callback. Clearer call sites, no ambient state.droppableboolean.isDroppable()checkspriority != priorityUndroppable.lockedMarkNotDroppablesets priority to the sentinel.selectwhen bothctx.Done()andreq.resultare ready requires explicit detection: the cancel path checks ifl.holder == reqafter acquiring the mutex, and if so callsreleaseInternalto prevent lock orphaning.Differences from the Python implementation
sync.Mutexon Lock guarding all stateasyncio.Future. Go uses bufferedchan error(cap 1) paired withsignal()that writes both an inspectableoutcomefield and the channelloop.call_later(), Go uses an injectedfunc(delayNs int64)callback. Lock owns the actualtime.AfterFuncand provides idempotencyasyncio.CancelledError. Go takescontext.Contextin Acquire with explicitlockedCancel/lockedRemovemethods__del__fallback. Go wraps Release insync.Oncetime.AfterFuncwith staleness guard viamaxAgeHolderpointer comparisonfunc(error)callbacks without mutex held, withrecover()for panic safetyTesting
98 tests across 4 test files:
All tests pass with
-race -count=5.AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :)