test(kv): address PR #745 round-1 review

Round-1 review on commit ad924ad:

- gemini medium: assert minimum elapsed time, not just upper bound.
  Without a lower bound a future regression returning DeadlineExceeded
  before actually waiting would silently pass. Added a lower-bound
  check with 200ms slack so a slow CI scheduler that trips ctx.Done()
  a hair before the wall clock catches up does not flake.

- claude bot (style): trim incident narrative from the
  verifyLeaderTimeout doc comment. Specific IPs, goroutine counts,
  and timestamps belong in the commit message and PR body, not the
  source — they go stale and add no design value at the call site.
  Kept the rationale (why 5s, the O(N) pendingReads feedback loop,
  match leaderForwardTimeout) and added a "see PR #745" reference
  for readers who want the full incident detail.

- claude bot: switch the regression test from stdlib
  `stderrors "errors"` to `github.com/cockroachdb/errors` for
  consistency with the project convention (CLAUDE.md mandates
  cockroachdb/errors at boundaries; the test is a boundary). Also
  drops the import alias which made gci complain about ordering.

- claude bot: add `if testing.Short() { t.Skip(...) }` guard so
  `go test -short ./kv/...` does not pay the verifyLeaderTimeout
  (5s) wait. Default `make test` still runs it.

- reviewdog/golangci (gci): import order fixed by reordering
  third-party (cockroachdb) above local (bootjp) per the project's
  default gci sections (standard, default, prefix(...)).

No behavior change. Test still passes:
  go test -race -count=1 -run TestVerifyLeaderEngine ./kv -- 6.06s
  go test -race -count=1 -short ./kv -- 5.55s (test skipped under -short)

No semantic change to the production fix; this commit is test +
comment polish only, so no caller audit needed.

Author: bootjp

kv/raft_engine.go

@@ -10,28 +10,20 @@ import (
 )
 
 // verifyLeaderTimeout caps how long the no-context verifyLeaderEngine path
-// is willing to wait for a ReadIndex round-trip.
-//
-// A previous version called engine.VerifyLeader with context.Background(),
-// so callers without an upstream deadline (LeaderProxy.Commit/Abort,
+// is willing to wait for a ReadIndex round-trip. Without this bound,
+// callers that hold context.Background() (LeaderProxy.Commit/Abort,
 // Coordinate.VerifyLeader, ShardedCoordinator.VerifyLeader[ForKey], and
 // the S3/SQS/admin /healthz/leader handlers) blocked indefinitely whenever
-// ReadIndex completion stalled — a single transient stall accumulated
-// callers permanently.
-//
-// Production hit this on 2026-05-08: a follower (192.168.0.214) lost its
-// network route mid-flight and the leader's ReadIndex completion stalled
-// intermittently. verifyLeaderEngine callers piled up at ~9/sec without
-// bound; after ~37 minutes the leader was holding 20,560 goroutines
-// (20,478 in submitRead select, oldest 39 minutes), CPU pinned at 1870%
-// (the Engine.run Ready loop walks pendingReads O(N) per tick, so the
-// queue feeds back on itself), and host MemAvailable trended toward 0
-// until OOM. The same pattern repeated on each new leader after failover.
+// ReadIndex completion stalled, and a single transient stall accumulated
+// callers permanently — Engine.run's Ready loop walks pendingReads O(N)
+// per tick, so the queue feeds back on itself once it grows.
 //
 // 5s matches leaderForwardTimeout: a verify that takes longer than a
-// single forward RPC is, by definition, useless as a freshness check,
-// and the proxy's verify-then-forward path stays within its 5s retry
-// budget.
+// single forward RPC is useless as a freshness check, and the proxy's
+// verify-then-forward path stays within its 5s retry budget.
+//
+// See PR #745 / incident 2026-05-08 for the goroutine-pile production
+// failure this prevents.
 const verifyLeaderTimeout = 5 * time.Second
 
 func engineForGroup(g *ShardGroup) raftengine.Engine {

kv/raft_engine_test.go

@@ -2,10 +2,11 @@ package kv
 
 import (
 	"context"
-	stderrors "errors"
 	"testing"
 	"time"
 
+	"github.com/cockroachdb/errors"
+
 	"github.com/bootjp/elastickv/internal/raftengine"
 )
 
@@ -16,8 +17,8 @@ import (
 // callers under test.
 type blockingLeaderView struct{}
 
-func (blockingLeaderView) State() raftengine.State        { return raftengine.StateLeader }
-func (blockingLeaderView) Leader() raftengine.LeaderInfo  { return raftengine.LeaderInfo{ID: "self"} }
+func (blockingLeaderView) State() raftengine.State       { return raftengine.StateLeader }
+func (blockingLeaderView) Leader() raftengine.LeaderInfo { return raftengine.LeaderInfo{ID: "self"} }
 func (blockingLeaderView) VerifyLeader(ctx context.Context) error {
 	<-ctx.Done()
 	return ctx.Err()
@@ -30,10 +31,17 @@ func (blockingLeaderView) LinearizableRead(ctx context.Context) (uint64, error)
 // TestVerifyLeaderEngine_BoundsBlockingReadIndex pins the regression: if a
 // stalled ReadIndex used to return only when the underlying ctx fired, but
 // callers passed context.Background(), the goroutine pinned forever. After
-// 2026-05-08-style stalls in production this must complete within roughly
+// the 2026-05-08 incident this must complete within roughly
 // verifyLeaderTimeout, surfacing context.DeadlineExceeded.
+//
+// Skipped under -short because the whole point is to wait for the deadline
+// to fire; the no-skip path adds verifyLeaderTimeout (5s) to every default
+// `make test` run.
 func TestVerifyLeaderEngine_BoundsBlockingReadIndex(t *testing.T) {
 	t.Parallel()
+	if testing.Short() {
+		t.Skip("skipping: blocks for verifyLeaderTimeout (5s)")
+	}
 
 	start := time.Now()
 	err := verifyLeaderEngine(blockingLeaderView{})
@@ -42,13 +50,26 @@ func TestVerifyLeaderEngine_BoundsBlockingReadIndex(t *testing.T) {
 	if err == nil {
 		t.Fatalf("verifyLeaderEngine(blocking) returned nil; expected DeadlineExceeded")
 	}
-	if !stderrors.Is(err, context.DeadlineExceeded) {
+	if !errors.Is(err, context.DeadlineExceeded) {
 		t.Fatalf("verifyLeaderEngine(blocking) err = %v; want DeadlineExceeded", err)
 	}
-	// Allow generous slack so a slow CI host does not flake; the point is
-	// not to assert a tight bound but to prove the call returns at all.
+	// Lower bound: confirm the engine actually held the call until the
+	// deadline fired. Without this, a future regression that returned
+	// DeadlineExceeded immediately (e.g. a misplaced ctx check before
+	// the engine call) would silently pass. Pulled in from gemini's
+	// PR #745 round-1 review — the upper bound alone proved bounded
+	// completion but not "the timeout actually fired."
+	//
+	// Tolerate a 200ms early-return slack so a slow CI scheduler that
+	// trips ctx.Done() a hair before the wall clock catches up does
+	// not flake.
+	const slack = 200 * time.Millisecond
+	if elapsed+slack < verifyLeaderTimeout {
+		t.Fatalf("verifyLeaderEngine(blocking) returned too early after %s; want >= %s (-%s slack)", elapsed, verifyLeaderTimeout, slack)
+	}
+	// Upper bound: prove the call returned at all. Generous so a slow
+	// CI host does not flake.
 	if elapsed > 2*verifyLeaderTimeout {
 		t.Fatalf("verifyLeaderEngine(blocking) returned after %s; want <= 2x verifyLeaderTimeout (%s)", elapsed, verifyLeaderTimeout)
 	}
 }
-