test(epaxos): resident-state conflict benchmarks - #8
Conversation
Reviewer's GuideReplace map-based conflict indexing with a per-lane conflictEngine, introduce folded-history record loading and executed-instance retirement, wire them into EPaxos core, and add resident-state benchmarks plus lint/test hardening. Sequence diagram for folded record load handshake via Ready.RecordLoadssequenceDiagram
actor Embedding
participant RawNode
participant Storage
Embedding->>RawNode: Step(message)
RawNode->>RawNode: needsRecordLoad(ref)
alt [needsRecordLoad returns true]
RawNode->>RawNode: deferRecordLoad(ref, message)
loop poll
Embedding->>RawNode: HasReady()
RawNode-->>Embedding: HasReady()
end
Embedding->>Storage: [load durable record for ref]
Storage-->>Embedding: RecordLoadResult
Embedding->>RawNode: ProvideRecordLoad(result)
RawNode->>RawNode: setInstanceRecord(inst, result.Record)
RawNode->>RawNode: maybeRefoldLoaded(ref)
RawNode->>RawNode: Step(deferred messages)
else [needsRecordLoad returns false]
RawNode->>RawNode: handlePrepare/handleEvidence
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 2 issues, and left some high level feedback:
- The
maxResidentInstancesguard is duplicated in bothProposeandProposeConfChange; consider factoring this into a small helper (e.g.checkResidentCapacity) so future callers don’t have to remember to reapply the same resident-count check. - The conflict engine’s
advanceFold(and related calls) panic when invariants are violated (e.g. non-contiguous folds); since this is library code, consider returning an error (or at least guarding at call sites and surfacing a typed error) instead of panicking so embeddings can handle corruption or misuse more gracefully. - In
deferRecordLoadyou sort and deduplicateReady.RecordLoadsfor every new ref; you could defer this work to the point where the Ready batch is frozen (or only normalize when the slice grows beyond 1 element) to reduce per-message overhead on high-load paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `maxResidentInstances` guard is duplicated in both `Propose` and `ProposeConfChange`; consider factoring this into a small helper (e.g. `checkResidentCapacity`) so future callers don’t have to remember to reapply the same resident-count check.
- The conflict engine’s `advanceFold` (and related calls) panic when invariants are violated (e.g. non-contiguous folds); since this is library code, consider returning an error (or at least guarding at call sites and surfacing a typed error) instead of panicking so embeddings can handle corruption or misuse more gracefully.
- In `deferRecordLoad` you sort and deduplicate `Ready.RecordLoads` for every new ref; you could defer this work to the point where the Ready batch is frozen (or only normalize when the slice grows beyond 1 element) to reduce per-message overhead on high-load paths.
## Individual Comments
### Comment 1
<location path="epaxos/node.go" line_range="4733-4666" />
<code_context>
+ })
+}
+
func (n *RawNode) computeAttrsAt(cmd Command, exclude InstanceRef, processAt uint64, timedPreAccept bool) Attributes {
conf := n.confFor(exclude.Conf)
deps := make([]InstanceNum, len(conf.Voters))
</code_context>
<issue_to_address>
**question (bug_risk):** Timed PreAccept now uses conflict-key–scoped walks instead of the previous broad lane scan; this changes dependency selection semantics and may need a deliberate review.
The old `timedPreAccept` path walked all resident instances and added eligible conflicts regardless of `ConflictKeys`. With the new `conflictEngine` integration, non-global commands now depend on `keyLaneSet`/`walkKeyDesc` keyed by `cmd.ConflictKeys`, and global commands on `maxEligibleAny`/`walkDesc`. This means TOQ-timed proposals now only depend on what the engine deems conflicting by key/global scope. Please confirm that all command types using timed PreAccept have complete `ConflictKeys` coverage, and that we don’t have workloads where partial/missing key tagging previously relied on the broader scan for safety.
</issue_to_address>
### Comment 2
<location path="epaxos/node.go" line_range="4653-4662" />
<code_context>
+// VisitConflicts yields resident in-flight instances that conflict with cmd.
+// Folded history is not enumerated. yield returns false to stop early.
+// The walk is designed to avoid heap allocation in the steady state.
+func (n *RawNode) VisitConflicts(cmd Command, yield func(InstanceRef, Status) bool) {
+ if n == nil || yield == nil || cmd.Kind == CommandNoop {
+ return
+ }
+ confID := n.currentHardState.Conf.ID
+ stop := false
+ visit := func(ref InstanceRef, status Status) bool {
+ if !yield(ref, status) {
+ stop = true
+ return false
+ }
+ return true
+ }
+ if commandHasGlobalConflictScope(cmd.Kind) {
+ n.engine.lanes(confID, func(lane instanceLane) bool {
+ if stop {
+ return false
</code_context>
<issue_to_address>
**suggestion (bug_risk):** VisitConflicts may return the same conflicting instance more than once due to overlapping key/global walks.
In the non-global path, `VisitConflicts` walks key-specific postings via `keyLaneSet`/`walkKeyDesc`, then all lanes again via `globalMax`/`walkGlobalDesc`, without deduplication. A resident with both key postings and global conflict scope (or shared lanes across keys) can therefore be yielded multiple times. If callers assume each `InstanceRef` appears only once, this can break embedding logic. Please either document that duplicates are expected or add a small `map[InstanceRef]struct{}` (when `cmd.Kind` != `CommandNoop`) to ensure each ref is yielded at most once.
Suggested implementation:
```golang
confID := n.currentHardState.Conf.ID
stop := false
// seen ensures that each conflicting instance is yielded at most once,
// even if it is discovered via overlapping walks (e.g., key-specific
// and global conflict scopes).
seen := make(map[InstanceRef]struct{})
visit := func(ref InstanceRef, status Status) bool {
if _, ok := seen[ref]; ok {
// Already yielded this instance; skip.
return true
}
seen[ref] = struct{}{}
if !yield(ref, status) {
stop = true
return false
}
return true
}
```
To guarantee de-duplication across all conflict walks, ensure that every place in `VisitConflicts` that currently calls `yield(ref, status)` instead calls `visit(ref, status)`. From the provided snippet this already appears to be the intended pattern, but if any direct `yield` calls remain later in the function, they should be replaced with `visit` so the `seen` map can filter duplicates.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| return true | ||
| } | ||
| if commandHasGlobalConflictScope(cmd.Kind) { |
There was a problem hiding this comment.
question (bug_risk): Timed PreAccept now uses conflict-key–scoped walks instead of the previous broad lane scan; this changes dependency selection semantics and may need a deliberate review.
The old timedPreAccept path walked all resident instances and added eligible conflicts regardless of ConflictKeys. With the new conflictEngine integration, non-global commands now depend on keyLaneSet/walkKeyDesc keyed by cmd.ConflictKeys, and global commands on maxEligibleAny/walkDesc. This means TOQ-timed proposals now only depend on what the engine deems conflicting by key/global scope. Please confirm that all command types using timed PreAccept have complete ConflictKeys coverage, and that we don’t have workloads where partial/missing key tagging previously relied on the broader scan for safety.
| func (n *RawNode) VisitConflicts(cmd Command, yield func(InstanceRef, Status) bool) { | ||
| if n == nil || yield == nil || cmd.Kind == CommandNoop { | ||
| return | ||
| } | ||
| confID := n.currentHardState.Conf.ID | ||
| stop := false | ||
| visit := func(ref InstanceRef, status Status) bool { | ||
| if !yield(ref, status) { | ||
| stop = true | ||
| return false |
There was a problem hiding this comment.
suggestion (bug_risk): VisitConflicts may return the same conflicting instance more than once due to overlapping key/global walks.
In the non-global path, VisitConflicts walks key-specific postings via keyLaneSet/walkKeyDesc, then all lanes again via globalMax/walkGlobalDesc, without deduplication. A resident with both key postings and global conflict scope (or shared lanes across keys) can therefore be yielded multiple times. If callers assume each InstanceRef appears only once, this can break embedding logic. Please either document that duplicates are expected or add a small map[InstanceRef]struct{} (when cmd.Kind != CommandNoop) to ensure each ref is yielded at most once.
Suggested implementation:
confID := n.currentHardState.Conf.ID
stop := false
// seen ensures that each conflicting instance is yielded at most once,
// even if it is discovered via overlapping walks (e.g., key-specific
// and global conflict scopes).
seen := make(map[InstanceRef]struct{})
visit := func(ref InstanceRef, status Status) bool {
if _, ok := seen[ref]; ok {
// Already yielded this instance; skip.
return true
}
seen[ref] = struct{}{}
if !yield(ref, status) {
stop = true
return false
}
return true
}To guarantee de-duplication across all conflict walks, ensure that every place in VisitConflicts that currently calls yield(ref, status) instead calls visit(ref, status). From the provided snippet this already appears to be the intended pattern, but if any direct yield calls remain later in the function, they should be replaced with visit so the seen map can filter duplicates.
![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 ProvideRecordLoad where a mismatch between the loaded record's Ref and the requested res.Ref could cause an infinite loop of record loads. Additionally, optimization opportunities were identified to avoid global map iterations in retireExecuted and to eliminate redundant heap allocations from unnecessary record cloning during apply and fold operations.
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.
| rec := res.Record | ||
| if rec.Ref != res.Ref && !res.Ref.IsZero() { | ||
| // allow Ref on result to define | ||
| if rec.Ref.IsZero() { | ||
| rec.Ref = res.Ref | ||
| } | ||
| } |
There was a problem hiding this comment.
If the loaded record's Ref does not match the requested res.Ref, installing it under rec.Ref while clearing res.Ref from pendingRecordLoads will cause the replayed messages for res.Ref to trigger another record load request. This can lead to an infinite loop of record loads.\n\nWe should strictly validate that the loaded record's Ref matches res.Ref (or populate it if it is zero) and return ErrInvalidRecord if they mismatch.
| rec := res.Record | |
| if rec.Ref != res.Ref && !res.Ref.IsZero() { | |
| // allow Ref on result to define | |
| if rec.Ref.IsZero() { | |
| rec.Ref = res.Ref | |
| } | |
| } | |
| rec := res.Record | |
| if rec.Ref.IsZero() { | |
| rec.Ref = res.Ref | |
| } | |
| if rec.Ref != res.Ref { | |
| return ErrInvalidRecord | |
| } |
| type item struct { | ||
| ref InstanceRef | ||
| rec InstanceRecord | ||
| } | ||
| byLane := make(map[instanceLane][]item) | ||
| for ref, inst := range n.instances { | ||
| if inst == nil || inst.rec.Status != StatusExecuted || !n.executed.contains(ref) { | ||
| continue | ||
| } | ||
| lane := laneFor(ref) | ||
| byLane[lane] = append(byLane[lane], item{ref: ref, rec: inst.rec}) | ||
| } | ||
| for lane, items := range byLane { | ||
| executedThrough := n.executed.prefix(lane) | ||
| if executedThrough == 0 { | ||
| continue | ||
| } | ||
| var target InstanceNum | ||
| if executedThrough > retain { | ||
| target = executedThrough - retain | ||
| } | ||
| folded := n.engine.foldedThrough(lane) | ||
| if target <= folded { | ||
| continue | ||
| } | ||
| for _, it := range items { | ||
| if it.ref.Instance <= folded || it.ref.Instance > target { | ||
| continue | ||
| } | ||
| if n.instances[it.ref] == nil { | ||
| continue | ||
| } | ||
| rec := it.rec.Clone() | ||
| n.engine.foldRecord(rec) | ||
| delete(n.instances, it.ref) | ||
| n.foldedInstances++ | ||
| } | ||
| contagious := folded | ||
| for next := folded + 1; next <= target; next++ { | ||
| if !n.engine.canAdvanceFold(lane, next) { | ||
| break | ||
| } | ||
| contagious = next | ||
| } | ||
| if contagious > folded { | ||
| n.engine.advanceFold(lane, contagious) | ||
| n.executed.forgetExactThrough(lane, contagious) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of retireExecuted iterates over all resident instances in n.instances on every call, which scales as Clone() on each retired record introduces unnecessary heap allocations.\n\nWe can optimize this by iterating over the active lanes in n.engine.laneIndex and checking only the sequential range of instances [folded + 1, target] for each lane. This avoids the global map iteration entirely and allows us to safely pass inst.rec directly to foldRecord without cloning.
for lane, index := range n.engine.laneIndex {
executedThrough := n.executed.prefix(lane)
if executedThrough == 0 {
continue
}
var target InstanceNum
if executedThrough > retain {
target = executedThrough - retain
}
folded := index.folded
if target <= folded {
continue
}
for instNum := folded + 1; instNum <= target; instNum++ {
ref := InstanceRef{Conf: lane.conf, Replica: lane.replica, Instance: instNum}
inst := n.instances[ref]
if inst == nil || inst.rec.Status != StatusExecuted || !n.executed.contains(ref) {
continue
}
n.engine.foldRecord(inst.rec)
delete(n.instances, ref)
n.foldedInstances++
}
contagious := folded
for next := folded + 1; next <= target; next++ {
if !n.engine.canAdvanceFold(lane, next) {
break
}
contagious = next
}
if contagious > folded {
n.engine.advanceFold(lane, contagious)
n.executed.forgetExactThrough(lane, contagious)
}
}| previous := inst.rec.Clone() | ||
| n.engine.apply(&previous, rec) |
There was a problem hiding this comment.
Calling inst.rec.Clone() here is unnecessary because n.engine.apply only reads from the previous record and does not modify or retain it. Passing &inst.rec directly avoids redundant heap allocations of slice fields.
| previous := inst.rec.Clone() | |
| n.engine.apply(&previous, rec) | |
| n.engine.apply(&inst.rec, rec) |
| rec := inst.rec.Clone() | ||
| n.engine.foldRecord(rec) |
There was a problem hiding this comment.
Calling inst.rec.Clone() here is unnecessary because n.engine.foldRecord only reads from the record and does not modify or retain it. Passing inst.rec directly avoids redundant heap allocations.
| rec := inst.rec.Clone() | |
| n.engine.foldRecord(rec) | |
| n.engine.foldRecord(inst.rec) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ac9a225e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MustSync bool | ||
| // RecordLoads requests durable InstanceRecords for folded refs referenced by | ||
| // inbound messages. Sorted and deduplicated; stable until Advance. | ||
| RecordLoads []InstanceRef |
There was a problem hiding this comment.
When a folded-ref Prepare/Evidence is deferred, the only work made visible to the embedding can be this new RecordLoads slice. Ready.Empty still ignores it, so HasReady/ReadyInto treat the node as idle and return an empty Ready instead of the load request, leaving the message stuck in pendingRecordLoads with no way for the application to call ProvideRecordLoad.
Useful? React with 👍 / 👎.
| target := n.readyTarget() | ||
| target.RecordLoads = append(target.RecordLoads, ref) |
There was a problem hiding this comment.
When deferRecordLoad runs while a Ready is already frozen, readyTarget() returns nextReady, so this appends the request to n.nextReady.RecordLoads. The later mergeNextReady path merges records/messages/committed work but never moves RecordLoads, so after the current Ready is advanced the load request disappears while the corresponding messages remain deferred in pendingRecordLoads.
Useful? React with 👍 / 👎.
| if rec.Ref != res.Ref && !res.Ref.IsZero() { | ||
| // allow Ref on result to define | ||
| if rec.Ref.IsZero() { | ||
| rec.Ref = res.Ref | ||
| } | ||
| } |
There was a problem hiding this comment.
If storage responds Found=true for request res.Ref but returns a record whose non-zero Record.Ref is different, this branch falls through: the checksum is validated for that other record, it is installed, and the wait for res.Ref is deleted before replaying messages for the original folded ref. That silently loses or re-defers the requested load instead of reporting an invalid result.
Useful? React with 👍 / 👎.
| len(rd.Messages) > len(n.frozenReady.Messages) || | ||
| len(rd.BootstrapMessages) > len(n.frozenReady.BootstrapMessages) || | ||
| len(rd.Committed) > len(n.frozenReady.Committed) || | ||
| len(rd.RecordLoads) > len(n.frozenReady.RecordLoads) || |
There was a problem hiding this comment.
For Ready batches that include RecordLoads alongside other work, Advance only checks the count and never verifies that the acknowledged refs match n.frozenReady.RecordLoads, unlike the equality checks for records/messages/committed entries below. A stale or out-of-order ack with the same length can consume the load request for a different folded ref, leaving the actual deferred messages waiting for a load the embedding was never asked to perform.
Useful? React with 👍 / 👎.
| } | ||
| rec := it.rec.Clone() | ||
| n.engine.foldRecord(rec) | ||
| delete(n.instances, it.ref) |
There was a problem hiding this comment.
After this deletes an executed instance from n.instances, delayed value-carrying messages for the same folded ref (PreAccept, Accept, TryPreAccept, or Commit) are treated as unknown because only the Prepare/Evidence handlers call needsRecordLoad. In a long partition after the retention tail moves, such a retry can install and persist a pre-executed/committed record over the durable executed record, resurrecting or downgrading an already applied instance.
Useful? React with 👍 / 👎.
| n.frozenReady.Messages = consumeReadyPrefix(n.frozenReady.Messages, len(rd.Messages)) | ||
| n.frozenReady.BootstrapMessages = consumeReadyPrefix(n.frozenReady.BootstrapMessages, len(rd.BootstrapMessages)) | ||
| n.frozenReady.Committed = consumeReadyPrefix(n.frozenReady.Committed, len(rd.Committed)) | ||
| n.frozenReady.RecordLoads = consumeReadyPrefix(n.frozenReady.RecordLoads, len(rd.RecordLoads)) |
There was a problem hiding this comment.
The new load requests are consumed here as soon as the caller advances the Ready, but the repository's Ready drivers I checked (for example examples/kv/cmd/kvnode/main.go's drainLocked) still only apply records, send messages, and then call Advance without loading these refs or calling ProvideRecordLoad. Any mixed Ready containing both normal work and RecordLoads will therefore drop the load request while the deferred messages remain stuck.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
c260e96 to
4b34960
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
BenchmarkComputeAttrsResident1k/100k, StepPreAcceptGrown, RetireExecuted on pre-grown nodes.
Summary by Sourcery
Introduce a per-lane conflict engine and executed-instance retirement with async record-load support, tighten validation and error handling across EPaxos and KV helpers, and add benchmarks, formal/TLA coverage, and stricter lint/CI gates.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: