kv(composed1): ABORT bypass + happy-path test + doc fix (round-3 on PR #895)

Three remaining items from claude's round-2 follow-up review + the
gemini HIGH-priority finding that was missed in round 2:

  * gemini HIGH (kv/fsm.go) — ABORT requests MUST bypass the
    Composed-1 gate.  If a route shifted between PREPARE and
    ABORT (or the observed version was evicted), a rejected
    ABORT would leave the txn's intent locks pinned in MVCC
    until LockResolver's TTL — minutes of write-blocked keys
    for what should be a one-RPC cleanup.

    Production callers (transaction.go, sharded_coordinator.go)
    happen to leave ObservedRouteVersion=0 on ABORT requests, so
    today the existing observedVer==0 short-circuit already
    protects them.  But that is enforcement-by-convention; a
    future ABORT construction site that copies a COMMIT struct
    by mistake would silently start failing aborts.

    Fix: add `if r.GetPhase() == pb.Phase_ABORT { return nil }`
    as the FIRST guard in verifyComposed1, before the
    routes/shardGroupID check.  Enforcement-in-code rather than
    enforcement-by-convention.

    Caller audit (the loop directive's semantic-change check):
    verifyComposed1 is only called from handleTxnRequest, which
    treats nil as success.  The ABORT bypass returns nil, so the
    caller's semantics are preserved.  No further wiring change.

  * claude follow-up #1 (distribution/engine.go) — The
    SetHistoryDepthForTest doc comment said "no internal
    synchronisation here" even though the function acquired
    e.mu.Lock.  Future readers would see the contradiction and
    not know which was authoritative.  Rewrote the comment to
    describe the actual contract: the write is lock-protected,
    but callers should still set depth before sharing the
    Engine with concurrent SnapshotAt/Current readers to avoid
    interleaving surprises around the eviction watermark.

  * claude follow-up #2 (kv/fsm_composed1_test.go) — The gate
    suite had no happy-path test.  An off-by-one in OwnerOf, a
    reversed comparison, or a sign-flip in the group-ID match
    would silently false-reject every valid commit and the
    failure-only suite would not catch it.  Added
    TestVerifyComposed1_ValidOwnershipPassesGate: this FSM
    serves the group that owns the key at BOTH the observed
    version AND the current catalog version, so the gate must
    return nil.

  * claude follow-up nit (kv/fsm_composed1_test.go) — The
    `//nolint:unparam` annotation on newComposed1FSM is now
    redundant: the new ABORT and shardGroupID=0 tests pass
    distinct shardGroupID values, so golangci-lint no longer
    flags the parameter as constant.  Removed (verified with
    `make lint` → 0 issues).

Also added TestVerifyComposed1_AbortPhaseSkipsGate as the
regression for the ABORT bypass: deliberately sets
ObservedRouteVersion=1 (non-zero) on the ABORT to defeat the
existing observedVer==0 short-circuit and isolate the new
Phase_ABORT guard.

Verification: go test -race -count=1 ./kv ./distribution → pass.
make lint → 0 issues.

Author: bootjp

distribution/engine.go

@@ -209,13 +209,15 @@ func (e *Engine) SnapshotAt(v uint64) (RouteHistorySnapshot, bool) {
 }
 
 // SetHistoryDepthForTest overrides the FIFO ring depth from outside
-// the package.  Test-only — callers MUST set the depth before the
-// Engine is shared with any concurrent reader (no internal
-// synchronisation here for the same reason TestEngineSnapshotAt_FIFOEviction
-// does the direct field write in-package; this seam exposes the
-// equivalent capability to external test packages that need a
-// small depth to exercise eviction without overwhelming TLC-style
-// bounded scenarios — claude review on PR #894).
+// the package.  Test-only.  Callers should set the depth before
+// sharing the Engine with concurrent SnapshotAt/Current readers to
+// avoid interleaving surprises around the eviction watermark, but
+// the write itself is lock-protected (e.mu.Lock below) so it is
+// safe to call from any goroutine that does not also expect a
+// consistent SnapshotAt view across the depth change.  Exists so
+// tests in the kv package can drive eviction-trigger scenarios
+// without adding a constructor option just for tests (claude
+// review on PR #894).
 //
 // Production code must use DefaultRouteHistoryDepth (32) or a
 // future operator-exposed config knob; this seam is build-time

kv/fsm.go

@@ -580,6 +580,20 @@ func (f *kvFSM) handleTxnRequest(ctx context.Context, r *pb.Request, commitTS ui
 // outside the ring (M4 retry), and ErrComposed1Violation wrapped
 // with per-key context otherwise.
 func (f *kvFSM) verifyComposed1(r *pb.Request) error {
+	// ABORT requests MUST always reach the abort handler so the
+	// txn's intent locks get released.  If a route shifted between
+	// PREPARE and ABORT (or the observed version was evicted), a
+	// rejected ABORT would leave the locks pinned until LockResolver
+	// noticed at TTL expiry — minutes of write-blocked keys for
+	// what should be a one-RPC cleanup.  Production callers carry
+	// ObservedRouteVersion=0 on ABORT (the existing observedVer==0
+	// check below handles that case), so this guard is defensive
+	// belt-and-suspenders: any future ABORT construction site that
+	// inadvertently sets the version still bypasses the gate
+	// (gemini HIGH on PR #895 — fail-open on ABORT).
+	if r.GetPhase() == pb.Phase_ABORT {
+		return nil
+	}
 	// Bypass the gate when EITHER the route history is unwired OR
 	// the FSM's shardGroupID is the unset sentinel (0).  Both
 	// fields are documented as "unset ⇒ short-circuit" but the

kv/fsm_composed1_test.go

@@ -28,11 +28,6 @@ func applyComposed1Snapshot(t *testing.T, e *distribution.Engine, version uint64
 // shard group ID the gate compares against.  Production wiring lives
 // in main.go's buildShardGroups; this helper short-circuits to the
 // test-only fixture without spinning up a real Raft group.
-//
-// the helper keeps it as a parameter so a future test can exercise
-// the "wrong-group" case without re-deriving the boilerplate.
-//
-//nolint:unparam // shardGroupID is currently always 1 in tests but
 func newComposed1FSM(t *testing.T, e *distribution.Engine, shardGroupID uint64) *kvFSM {
 	t.Helper()
 	fsmIface := NewKvFSMWithHLC(store.NewMVCCStore(), NewHLC(),
@@ -256,3 +251,73 @@ func TestVerifyComposed1_ShardGroupIDZeroSkipsGate(t *testing.T) {
 			"shardGroupID=0 must bypass before any ring lookup, so ErrComposed1VersionGCd must not surface either")
 	}
 }
+
+// TestVerifyComposed1_ValidOwnershipPassesGate is the happy-path
+// witness the gate suite was missing (claude follow-up review on
+// PR #895): when this FSM serves the group that owns the key at
+// BOTH the txn's observed version AND the current catalog version,
+// verifyComposed1 returns nil.  Without this test an off-by-one
+// in OwnerOf, a reversed comparison, or an inadvertent sign-flip
+// in the group-ID match would silently false-reject every valid
+// commit and the failure-only suite would not catch it.
+func TestVerifyComposed1_ValidOwnershipPassesGate(t *testing.T) {
+	t.Parallel()
+
+	// At v=1 AND v=2, key "k" is owned by group 1.  An FSM serving
+	// group 1 must allow a commit pinned at v=1 to pass both the
+	// observed-version check (routes[1][k] = g1) and the
+	// current-version fence (routes[2][k] = g1).
+	e := distribution.NewEngine()
+	applyComposed1Snapshot(t, e, 1, []distribution.RouteDescriptor{
+		{RouteID: 100, Start: []byte(""), End: nil, GroupID: 1, State: distribution.RouteStateActive},
+	})
+	applyComposed1Snapshot(t, e, 2, []distribution.RouteDescriptor{
+		{RouteID: 101, Start: []byte(""), End: nil, GroupID: 1, State: distribution.RouteStateActive},
+	})
+	fsm := newComposed1FSM(t, e, 1)
+
+	require.NoError(t, fsm.verifyComposed1(commitTxnRequest(1, "k")),
+		"verifyComposed1 must return nil when this FSM's shardGroupID matches the owner at BOTH the observed version and the current catalog version — covers the happy path neither the failure tests nor the bypass tests exercise")
+}
+
+// TestVerifyComposed1_AbortPhaseSkipsGate is the regression for the
+// gemini HIGH finding on PR #895: ABORT requests MUST bypass the
+// Composed-1 gate so that a route shift between PREPARE and ABORT
+// cannot strand the txn's intent locks.  Without the early
+// Phase_ABORT short-circuit, an ABORT that arrives at the wrong
+// group after a MoveRange would fail with ErrComposed1Violation and
+// the locks would remain pinned until LockResolver's TTL — minutes
+// of write-blocked keys for what should be a one-RPC cleanup.
+//
+// The production callers (transaction.go and sharded_coordinator.go)
+// happen to leave ObservedRouteVersion=0 on ABORT, so today the
+// existing observedVer==0 branch already protects them, but that's
+// enforcement-by-convention.  The Phase_ABORT guard is
+// enforcement-in-code: a future ABORT construction site that
+// inadvertently copies a COMMIT request struct would still bypass.
+func TestVerifyComposed1_AbortPhaseSkipsGate(t *testing.T) {
+	t.Parallel()
+
+	// Wire the engine so that, absent the ABORT bypass, the gate
+	// would reject: key "k" is owned by group 5 at v=1 but the
+	// FSM serves group 1.  The test deliberately sets
+	// ObservedRouteVersion=1 (non-zero) on the ABORT to defeat the
+	// existing observedVer==0 short-circuit and isolate the
+	// Phase_ABORT guard.
+	e := distribution.NewEngine()
+	applyComposed1Snapshot(t, e, 1, []distribution.RouteDescriptor{
+		{RouteID: 100, Start: []byte(""), End: nil, GroupID: 5, State: distribution.RouteStateActive},
+	})
+	fsm := newComposed1FSM(t, e, 1)
+
+	abortReq := &pb.Request{
+		IsTxn:                true,
+		Phase:                pb.Phase_ABORT,
+		ObservedRouteVersion: 1,
+		Mutations: []*pb.Mutation{
+			{Op: pb.Op_DEL, Key: []byte("k")},
+		},
+	}
+	require.NoError(t, fsm.verifyComposed1(abortReq),
+		"ABORT requests must bypass the Composed-1 gate even with a non-zero ObservedRouteVersion pinned, so a route shift between PREPARE and ABORT cannot strand the txn's intent locks past lock-resolver TTL")
+}