perf(epaxos): integrate conflict engine; remove O(n) index#3
Merged
Conversation
Drop govet enable-all; keep revive/gosec/exhaustive enabled for R9.
Make epaxos package lint-clean under the U1 strict config: errcheck on Tick paths, revive renames, intentional test nolints for G115/G304/exhaustive subsets, and leave production wire G115 checks intact.
Root golangci-lint does not load the nested module; add a second CI step and document the local dual-module lint commands in AGENTS.md.
Mirror parser.uvarint8 for bootstrap envelopes so oversized wire types cannot truncate into the valid BootstrapMessageType band.
Re-check depsCount against maxWireDeps before int conversion and arena slicing so a hostile evidence frame cannot panic the node.
MkdirAll/WriteFile already use restrictive 0o750/0o600; the G115 suppressions were false and would hide future mode regressions.
Standalone per-lane sparse radix max-seq index and per-key postings with fold summaries; property tests prove oracle equivalence and fold domination.
Wire RawNode through setInstanceRecord/installInstance, rewrite computeAttrsAt and tryPreAcceptConflict over key/global posting walks, and delete the old conflict maps with zero residue.
walkGlobalDesc no longer scans unrelated residents; subtrees with globalMax==0 are skipped. Add sparse-gap visit-counter regression test.
Reviewer's GuideReplaces the legacy O(n) conflict indexing in the EPaxos RawNode with a new per-lane conflictEngine, rewrites attribute and conflict computations to use it (including TOQ paths), introduces folding/retirement of executed instances, tightens validation, error handling, and tests, and wires stricter linting plus CI for golangci-lint. Sequence diagram for TryPreAccept conflict detection with conflictEnginesequenceDiagram
actor Client
participant RawNode
participant conflictEngine
participant Instances as instances[InstanceRef]
Client->>RawNode: propose / TryPreAccept(Message)
RawNode->>RawNode: tryPreAcceptConflict(m)
opt commandHasGlobalConflictScope
RawNode->>conflictEngine: lanes(m.Ref.Conf, yield)
loop per lane
conflictEngine-->>RawNode: maxEligibleAny(lane)
RawNode->>conflictEngine: walkDesc(lane, resident, yield)
conflictEngine-->>RawNode: (instance, laneSlot)
RawNode->>Instances: lookup InstanceRef
RawNode->>RawNode: considerRecord(ref, instance)
end
end
opt key-scoped conflicts
RawNode->>conflictEngine: keyLaneSet(m.Ref.Conf, Command.ConflictKeys, yield)
loop per lane/key
conflictEngine-->>RawNode: keyMax(conf, key, lane)
RawNode->>conflictEngine: walkKeyDesc(conf, key, lane, resident, yield)
conflictEngine-->>RawNode: (instance, laneSlot)
RawNode->>Instances: lookup InstanceRef
RawNode->>RawNode: considerRecord(ref, instance)
end
RawNode->>conflictEngine: lanes(m.Ref.Conf, yield)
loop global lanes
conflictEngine-->>RawNode: globalMax(lane)
RawNode->>conflictEngine: walkGlobalDesc(lane, resident, yield)
conflictEngine-->>RawNode: (instance, laneSlot)
RawNode->>Instances: lookup InstanceRef
RawNode->>RawNode: considerRecord(ref, instance)
end
end
RawNode-->>Client: conflictRef, conflictStatus, found
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
Closes #10 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the O(all instances) conflict and attributes machinery with a per-lane conflict engine using radix trees and posting sets to track resident and retired instances. It integrates this engine into RawNode to optimize attribute computation and conflict checks, while also enforcing strict linting. The review comments are highly constructive and should be kept: they suggest removing unnecessary pointer indirection on the keyLanes map to reduce allocations, and optimizing descending traversals in computeAttrsAt to terminate early once the maximum conflicting instance is found.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| type conflictEngine struct { | ||
| laneIndex map[instanceLane]*laneIndex | ||
| byKey map[ConfID]map[string]*keyLanes |
There was a problem hiding this comment.
The byKey map is defined as map[ConfID]map[string]*keyLanes, where keyLanes is itself a map type (map[instanceLane]*keyLane). In Go, maps are reference types, so using a pointer to a map (*keyLanes) introduces unnecessary indirection and an extra allocation per unique key during insertion. Changing byKey to use keyLanes directly (i.e., map[ConfID]map[string]keyLanes) would make the code more idiomatic, improve readability, and reduce allocations.
Suggested change
| byKey map[ConfID]map[string]*keyLanes | |
| byKey map[ConfID]map[string]keyLanes |
Comment on lines
+4515
to
+4569
| walkKey := func(lane instanceLane, key []byte, from InstanceNum) { | ||
| if from == 0 || lane.conf != exclude.Conf { | ||
| return | ||
| } | ||
| n.engine.walkKeyDesc(exclude.Conf, key, lane, from, func(instance InstanceNum, slot laneSlot) bool { | ||
| if !slot.eligible() { | ||
| return true | ||
| } | ||
| ref := InstanceRef{Conf: lane.conf, Replica: lane.replica, Instance: instance} | ||
| inst := n.instances[ref] | ||
| if inst != nil && addRef(ref, inst.rec) { | ||
| continue | ||
| if inst == nil { | ||
| return true | ||
| } | ||
| addFallbackLane(lane) | ||
| } | ||
| _ = addRef(ref, inst.rec) | ||
| // continue: need max across candidates; do not stop at first | ||
| return true | ||
| }) | ||
| } | ||
| if cmd.Kind == CommandNoop { | ||
| return Attributes{Seq: seq, Deps: deps} | ||
| walkGlobal := func(lane instanceLane, from InstanceNum) { | ||
| if from == 0 || lane.conf != exclude.Conf { | ||
| return | ||
| } | ||
| n.engine.walkGlobalDesc(lane, from, func(instance InstanceNum, _ laneSlot) bool { | ||
| ref := InstanceRef{Conf: lane.conf, Replica: lane.replica, Instance: instance} | ||
| inst := n.instances[ref] | ||
| if inst == nil { | ||
| addRetired(lane, instance) | ||
| return true | ||
| } | ||
| _ = addRef(ref, inst.rec) | ||
| return true | ||
| }) | ||
| } | ||
|
|
||
| if commandHasGlobalConflictScope(cmd.Kind) { | ||
| addIndexedLanes(n.allConflicts) | ||
| if len(n.allConflicts) == 0 { | ||
| for ref, inst := range n.instances { | ||
| if inst != nil { | ||
| addRef(ref, inst.rec) | ||
| } | ||
| } | ||
| } | ||
| } else if timedPreAccept { | ||
| for ref, inst := range n.instances { | ||
| if inst != nil { | ||
| addRef(ref, inst.rec) | ||
| n.engine.lanes(exclude.Conf, func(lane instanceLane) bool { | ||
| resident, retired := n.engine.maxEligibleAny(lane) | ||
| // Global-scope commands depend on any eligible conflict, not only global-flagged records. | ||
| if resident != 0 { | ||
| n.engine.walkDesc(lane, resident, func(instance InstanceNum, slot laneSlot) bool { | ||
| if !slot.eligible() { | ||
| return true | ||
| } | ||
| ref := InstanceRef{Conf: lane.conf, Replica: lane.replica, Instance: instance} | ||
| inst := n.instances[ref] | ||
| if inst == nil { | ||
| return true | ||
| } | ||
| _ = addRef(ref, inst.rec) | ||
| return true | ||
| }) | ||
| } | ||
| } | ||
| addRetired(lane, retired) | ||
| return true | ||
| }) |
There was a problem hiding this comment.
In computeAttrsAt, walkKey and walkGlobal traverse the conflict engine's postings and global lists in descending order of InstanceNum to find the maximum conflicting instance for each lane. Since the lists are traversed in descending order, the first conflicting instance encountered is guaranteed to have the largest InstanceNum for that lane. Once a dependency is successfully added via addRef (or addRetired), any subsequent instances in the same lane will have smaller instance numbers and cannot increase deps[idx]. We can optimize these walks to terminate early by returning false from the yield function as soon as a dependency is found, avoiding unnecessary traversal of the remaining elements in the list.
walkKey := func(lane instanceLane, key []byte, from InstanceNum) {
if from == 0 || lane.conf != exclude.Conf {
return
}
n.engine.walkKeyDesc(exclude.Conf, key, lane, from, func(instance InstanceNum, slot laneSlot) bool {
if !slot.eligible() {
return true
}
ref := InstanceRef{Conf: lane.conf, Replica: lane.replica, Instance: instance}
inst := n.instances[ref]
if inst == nil {
return true
}
return !addRef(ref, inst.rec)
})
}
walkGlobal := func(lane instanceLane, from InstanceNum) {
if from == 0 || lane.conf != exclude.Conf {
return
}
n.engine.walkGlobalDesc(lane, from, func(instance InstanceNum, _ laneSlot) bool {
ref := InstanceRef{Conf: lane.conf, Replica: lane.replica, Instance: instance}
inst := n.instances[ref]
if inst == nil {
addRetired(lane, instance)
return false
}
return !addRef(ref, inst.rec)
})
}
if commandHasGlobalConflictScope(cmd.Kind) {
n.engine.lanes(exclude.Conf, func(lane instanceLane) bool {
resident, retired := n.engine.maxEligibleAny(lane)
// Global-scope commands depend on any eligible conflict, not only global-flagged records.
if resident != 0 {
n.engine.walkDesc(lane, resident, func(instance InstanceNum, slot laneSlot) bool {
if !slot.eligible() {
return true
}
ref := InstanceRef{Conf: lane.conf, Replica: lane.replica, Instance: instance}
inst := n.instances[ref]
if inst == nil {
return true
}
return !addRef(ref, inst.rec)
})
}
addRetired(lane, retired)
return true
})![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/ci.yml" line_range="55-64" />
<code_context>
chmod +x /tmp/lein
echo /tmp >> "$GITHUB_PATH"
+ - name: Run golangci-lint
+ uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9
+ with:
+ version: v2.12.2
+
+ - name: Run golangci-lint (examples/kv)
+ uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9
+ with:
+ version: v2.12.2
+ working-directory: examples/kv
+
- name: Run repository verification gates
</code_context>
<issue_to_address>
**issue (bug_risk):** golangci-lint configuration and CI Go version may not match supported tooling versions
`.golangci.yml` sets `run.go: "1.26"`, but that Go version does not exist. golangci-lint will try to use this to pick a toolchain, which can cause failures or unexpected defaults, especially with `golangci-lint-action@v2.12.2` that may not recognize it. Please set `run.go` to an actually supported Go version that matches the repo and CI toolchain so lint runs remain stable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+55
to
+64
| - name: Run golangci-lint | ||
| uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9 | ||
| with: | ||
| version: v2.12.2 | ||
|
|
||
| - name: Run golangci-lint (examples/kv) | ||
| uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9 | ||
| with: | ||
| version: v2.12.2 | ||
| working-directory: examples/kv |
There was a problem hiding this comment.
issue (bug_risk): golangci-lint configuration and CI Go version may not match supported tooling versions
.golangci.yml sets run.go: "1.26", but that Go version does not exist. golangci-lint will try to use this to pick a toolchain, which can cause failures or unexpected defaults, especially with golangci-lint-action@v2.12.2 that may not recognize it. Please set run.go to an actually supported Go version that matches the repo and CI toolchain so lint runs remain stable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Solution / proposal
Wire RawNode through setInstanceRecord; rewrite computeAttrsAt/tryPreAcceptConflict over key/global walks; prune global walks via globalMax; delete old conflict maps (residue empty).
Closes #
Summary by Sourcery
Integrate a per-lane conflict engine for EPaxos, retire the legacy conflict indexes, and tighten safety, testing, and tooling around conflict handling, timing, and checkpoints.
Enhancements:
Build:
CI:
Documentation:
Tests:
Closes #10