Skip to content

Commit 772a7da

Browse files
committed
feat(keyviz): address PR #822 round-1 (Gemini HIGH ×2 + Codex P2 + Gemini MEDIUM)
Three review items, four bot comments (two duplicates): - Gemini HIGH (line 650) + Gemini HIGH (line 679) + Codex P2 (line 654) — all the same regression: addWrite called updateFallbackState(value) without passing the contributor's (group, term), so resolveWrite's fallback path always returned (0, 0) for the identity slot even when a known-term source supplied the winning max value (e.g. known=100 + legacy=50 returned RaftGroupIDs=[0]). This regressed the PR-3b per-cell identity contract for mixed-version cells. Fixed: updateFallbackState now takes (value, group, term) and stamps lastGroup/lastTerm when value >= fallbackMax (last-touched tie-break matching addRead). When the known-term contributor wins, its identity propagates to the merged cell as expected; when the legacy contributor wins, identity correctly resets to (0, 0) (TestMergeKeyVizMatrices...OnUnknownTerm pins this case). - Gemini MEDIUM (line 780) — hasConflict loop over termConflict was wasteful. recordTermContribution only ever stores `true` (entries are never reset), so `len(c.termConflict) > 0` is equivalent and clearer. Caller audit (per loop instruction, updateFallbackState signature change is a semantic shift on its parameter list): - updateFallbackState: internal helper, only caller is addWrite (same file, updated). No external consumer. - hasConflict: behavior unchanged (length check is a pure optimization of the previous loop), no caller impact. New test: TestMergeKeyVizMatricesFallbackPreservesKnownTermWinnerIdentity mirrors the existing fallback test but flips who wins — modern (100, group=7, term=42) + legacy (50, no arrays) → fallback returns value=100 with identity=(7, 42). The prior PR-3c diff would have emitted (0, 0) and silently dropped the modern source's per-cell identity in mixed-version clusters.
1 parent c1aa23b commit 772a7da

2 files changed

Lines changed: 58 additions & 11 deletions

File tree

internal/admin/keyviz_fanout.go

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ func (c *cellMergeAcc) addWrite(value, group, term uint64) {
647647
if value == 0 {
648648
return
649649
}
650-
c.updateFallbackState(value)
650+
c.updateFallbackState(value, group, term)
651651
// LeaderTerm == 0 (or group == 0) means "term not tracked" —
652652
// the documented sentinel from a legacy peer or a publisher
653653
// that has not yet fired. We CANNOT safely sum across an
@@ -664,10 +664,19 @@ func (c *cellMergeAcc) addWrite(value, group, term uint64) {
664664
// updateFallbackState advances the fallback-max accounting that
665665
// applies when hasUnknownTerm is set at resolve time. Maintained for
666666
// every non-zero contribution so a cell that later flips into the
667-
// fallback path has the correct running max + disagreement signal.
668-
func (c *cellMergeAcc) updateFallbackState(value uint64) {
669-
if value > c.fallbackMax {
667+
// fallback path has the correct running max + disagreement signal
668+
// AND the identity of the contributor that supplied the max value.
669+
// Tie-break is last-touched (matching addRead) so a cell with two
670+
// equal contributions keeps the most recently processed identity.
671+
// Without the identity tracking, a cell with one known-term source
672+
// + one unknown-term source would always fall back to (0, 0) even
673+
// when the known-term source supplied the winning max (Gemini HIGH
674+
// + Codex P2 on PR #822).
675+
func (c *cellMergeAcc) updateFallbackState(value, group, term uint64) {
676+
if value >= c.fallbackMax {
670677
c.fallbackMax = value
678+
c.lastGroup = group
679+
c.lastTerm = term
671680
}
672681
if c.fallbackSeenNonZeroValue == 0 {
673682
c.fallbackSeenNonZeroValue = value
@@ -767,17 +776,15 @@ func (c *cellMergeAcc) resolveWrite() (value, group, term uint64) {
767776

768777
// hasConflict returns true when any (group, term) at this cell saw
769778
// disagreement, or — under the fallback path — when ≥2 distinct
770-
// non-zero values were reported.
779+
// non-zero values were reported. recordTermContribution only ever
780+
// stores `true` in termConflict (entries are never reset), so a
781+
// length check is equivalent to scanning the map (Gemini MEDIUM
782+
// on PR #822).
771783
func (c *cellMergeAcc) hasConflict() bool {
772784
if c.hasUnknownTerm {
773785
return c.fallbackNonZeroDistinct
774786
}
775-
for k := range c.termConflict {
776-
if c.termConflict[k] {
777-
return true
778-
}
779-
}
780-
return false
787+
return len(c.termConflict) > 0
781788
}
782789

783790
// unionColumns returns the sorted union of column timestamps across

internal/admin/keyviz_fanout_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,46 @@ func TestMergeKeyVizMatricesGroupTermFallbackOnUnknownTerm(t *testing.T) {
236236
require.True(t, merged.Rows[0].Conflict, "fallback path raises conflict when ≥2 distinct non-zero values were seen")
237237
}
238238

239+
// TestMergeKeyVizMatricesFallbackPreservesKnownTermWinnerIdentity
240+
// pins the Gemini HIGH / Codex P2 fix on PR #822: when the
241+
// fallback path is triggered by an unknown-term contribution but
242+
// the KNOWN-term source supplies the winning max value, the
243+
// merged identity must reflect that known-term contributor —
244+
// NOT fall back to (0, 0). Mirror image of
245+
// TestMergeKeyVizMatricesGroupTermFallbackOnUnknownTerm where the
246+
// legacy peer wins; this test pins the case the prior PR-3c diff
247+
// regressed (identity was always 0 under fallback regardless of
248+
// who won).
249+
func TestMergeKeyVizMatricesFallbackPreservesKnownTermWinnerIdentity(t *testing.T) {
250+
t.Parallel()
251+
col := []int64{1_700_000_000_000}
252+
// Modern source wins with 100.
253+
modern := KeyVizMatrix{
254+
ColumnUnixMs: col,
255+
Series: keyVizSeriesWrites,
256+
Rows: []KeyVizRow{
257+
{BucketID: "route:9", Values: []uint64{100}, RaftGroupIDs: []uint64{7}, LeaderTerms: []uint64{42}},
258+
},
259+
}
260+
// Legacy peer reports 50 with no arrays — triggers fallback but
261+
// loses the max.
262+
legacy := KeyVizMatrix{
263+
ColumnUnixMs: col,
264+
Series: keyVizSeriesWrites,
265+
Rows: []KeyVizRow{
266+
{BucketID: "route:9", Values: []uint64{50}},
267+
},
268+
}
269+
merged := mergeKeyVizMatrices([]KeyVizMatrix{modern, legacy}, keyVizSeriesWrites)
270+
require.Len(t, merged.Rows, 1)
271+
require.Equal(t, []uint64{100}, merged.Rows[0].Values, "fallback max-merge keeps the larger value")
272+
require.Equal(t, []uint64{7}, merged.Rows[0].RaftGroupIDs,
273+
"identity must reflect the known-term contributor that supplied the winning max — not (0, 0)")
274+
require.Equal(t, []uint64{42}, merged.Rows[0].LeaderTerms,
275+
"same: known-term contributor's term wins because its value won the fallback max")
276+
require.True(t, merged.Rows[0].Conflict, "100 != 50 disagreement raises the conflict flag under fallback")
277+
}
278+
239279
// TestMergeKeyVizMatricesPerCellIdentityMatchesValueOwner pins the
240280
// Gemini MEDIUM fix: when maxMerge picks a value from one source,
241281
// the identity at that cell must come from the SAME source — not

0 commit comments

Comments
 (0)