distribution(composed1): break+compact+rename per claude review on PR #894

Three findings from claude's review of PR #894, all valid:

  * OwnerOf O(N)-vs-O(early-break) on sorted route list.  Routes
    are sorted by Start (UpdateRoute / routesFromCatalog keep them
    sorted; recordHistorySnapshotLocked clones from that order).
    Once key < r.Start, no later route can cover key either.
    `continue` worked but iterated the whole tail; `break` short-
    circuits.  Matters because M3 puts OwnerOf on every txn
    commit's apply path.

  * historyOrder backing-array growth on FIFO eviction.  The
    previous `e.historyOrder = e.historyOrder[1:]` only moved the
    slice header — the head of the original backing array stayed
    alive across evictions, growing unboundedly.  Replace with an
    explicit `make([]uint64, len-1, historyDepth) + copy` so the
    array stays bounded at historyDepth.

  * recordHistorySnapshot lock contract fragility.  The function
    requires the caller to hold e.mu (write lock); the doc said
    so but nothing in the name signalled it.  Renamed to
    recordHistorySnapshotLocked per the Go convention so a future
    refactor that moves the call past an Unlock surfaces the
    contract by name.

Test nits also addressed:

  * TestEngineSnapshotAt_FIFOEviction now calls t.Parallel() and
    the direct `e.historyDepth = 3` write is documented as
    test-local (Engine not yet shared with any concurrent
    reader).
  * TestKvFSM_WithRouteHistory_NilProviderTreatedAsUnwired asserts
    shardGroupID=7 explicitly so a future refactor that
    accidentally zeroes it would be caught.

Verification: go test -race -count=1 ./distribution ./kv → pass
(1.0 s + 8.1 s).  No spec change; no contract change visible to
callers outside the distribution package.

Author: bootjp

distribution/engine.go

@@ -80,7 +80,7 @@ func NewEngine() *Engine {
 func NewEngineWithDefaultRoute() *Engine {
 	engine := NewEngine()
 	engine.UpdateRoute([]byte(""), nil, defaultGroupID)
-	engine.recordHistorySnapshot()
+	engine.recordHistorySnapshotLocked()
 	return engine
 }
 
@@ -135,7 +135,7 @@ func (e *Engine) ApplySnapshot(snapshot CatalogSnapshot) error {
 
 	e.routes = routes
 	e.catalogVersion = snapshot.Version
-	e.recordHistorySnapshot()
+	e.recordHistorySnapshotLocked()
 	return nil
 }
 
@@ -157,10 +157,19 @@ func (s RouteHistorySnapshot) Version() uint64 { return s.version }
 // pre-bootstrap state or an explicitly-uncovered range).  Mirrors
 // Engine.GetRoute's right-half-open interval semantics but against
 // the historical snapshot, not the live engine state.
+//
+// Routes are sorted by Start (recordHistorySnapshotLocked clones from
+// e.routes, which Engine.UpdateRoute / routesFromCatalog keep sorted),
+// so the scan can break the moment key < r.Start — every later route
+// has a strictly greater Start and cannot cover key either.  This
+// matters because M3 puts OwnerOf on every txn commit's apply path
+// (claude review on PR #894 — break-vs-continue lifts the worst-case
+// scan from O(N) to "first non-covering gap" without changing the
+// resolution semantics).
 func (s RouteHistorySnapshot) OwnerOf(key []byte) (uint64, bool) {
 	for _, r := range s.routes {
 		if bytes.Compare(key, r.Start) < 0 {
-			continue
+			break
 		}
 		if r.End != nil && bytes.Compare(key, r.End) >= 0 {
 			continue
@@ -191,12 +200,15 @@ func (e *Engine) HistoryDepth() int {
 	return e.historyDepth
 }
 
-// recordHistorySnapshot pushes the current (catalogVersion, routes)
-// pair into the ring.  Caller MUST hold e.mu (write lock) — invoked
-// from ApplySnapshot under the write lock, and from
-// NewEngineWithDefaultRoute before the Engine is shared with any
-// concurrent reader.  Idempotent on re-record at the same version.
-func (e *Engine) recordHistorySnapshot() {
+// recordHistorySnapshotLocked pushes the current (catalogVersion,
+// routes) pair into the ring.  The `Locked` suffix is the Go
+// convention for "caller MUST hold the receiver's lock" — checked
+// by reviewers via name, not by the runtime (claude review on PR
+// #894 — fragile lock contract).  Invoked from ApplySnapshot under
+// the write lock and from NewEngineWithDefaultRoute before the
+// Engine is shared with any concurrent reader.  Idempotent on
+// re-record at the same version.
+func (e *Engine) recordHistorySnapshotLocked() {
 	if e.history == nil {
 		// Engines constructed via the bare struct literal (e.g.
 		// internal test seams) — no history ring configured.  Skip
@@ -210,7 +222,17 @@ func (e *Engine) recordHistorySnapshot() {
 	}
 	if len(e.historyOrder) >= e.historyDepth {
 		evict := e.historyOrder[0]
-		e.historyOrder = e.historyOrder[1:]
+		// Copy the retained tail into fresh storage rather than
+		// reslicing.  `historyOrder[1:]` only advances the slice
+		// header — the head of the original backing array stays
+		// alive and grows unboundedly across evictions.  At depth=32
+		// this is small, but the FIFO eviction is the only place
+		// the array grows, and the compaction is free (single
+		// allocation, single copy of <=historyDepth entries —
+		// claude review on PR #894 — backing-array leak).
+		retained := make([]uint64, len(e.historyOrder)-1, e.historyDepth)
+		copy(retained, e.historyOrder[1:])
+		e.historyOrder = retained
 		delete(e.history, evict)
 	}
 	cloned := make([]Route, len(e.routes))

distribution/engine_test.go

@@ -559,8 +559,14 @@ func TestEngineSnapshotAt_PreservesHistoryAcrossVersions(t *testing.T) {
 // test pins the eviction order so a future depth change does not
 // silently break the M3 contract.
 func TestEngineSnapshotAt_FIFOEviction(t *testing.T) {
+	t.Parallel()
 	e := NewEngine()
-	// Force a tiny depth so the test is bounded and explicit.
+	// Force a tiny depth so the test is bounded and explicit.  The
+	// direct field write is safe because `e` is local to this test
+	// goroutine and the depth is set before any ApplySnapshot fires;
+	// once the Engine is published to concurrent readers, the depth
+	// would have to flow through a constructor option (claude review
+	// on PR #894 — fragile-but-test-local lock contract).
 	e.historyDepth = 3
 
 	for v := uint64(1); v <= 5; v++ {

kv/fsm_route_history_test.go

@@ -92,6 +92,9 @@ func TestKvFSM_WithRouteHistory_NilProviderTreatedAsUnwired(t *testing.T) {
 	// shardGroupID is still recorded — the design doc says zero is
 	// the "unset" sentinel but a caller that explicitly passes a
 	// non-zero shardGroupID with a nil provider gets the gate
-	// short-circuited anyway via the nil routes check.  No invariant
-	// to assert on the ID here.
+	// short-circuited anyway via the nil routes check.  Assert the
+	// non-zero ID survives so a future refactor that accidentally
+	// zeroes it would be caught (claude review on PR #894).
+	require.Equal(t, uint64(7), fsm.shardGroupID,
+		"WithRouteHistory(nil, 7) must still record shardGroupID=7; the M3 nil-routes short-circuit fires before the ID is consulted")
 }