Skip to content

NRG: Invalidate pae cache on WAL truncate and snapshot install - #8149

Merged
neilalexander merged 2 commits into
mainfrom
maurice/truncate-pae
May 13, 2026
Merged

NRG: Invalidate pae cache on WAL truncate and snapshot install#8149
neilalexander merged 2 commits into
mainfrom
maurice/truncate-pae

Conversation

@MauriceVanVeen

Copy link
Copy Markdown
Member

n.pae caches append entries and is normally drained by applyCommit. Three paths bypassed that drain: truncateWAL, installSnapshot, and catchup-from-snapshot. Memory would leak (bounded by paeDropThreshold) and the cache would become less efficient, requiring more disk loads until a stepdown or server restart.

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@MauriceVanVeen
MauriceVanVeen requested a review from a team as a code owner May 13, 2026 10:18
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 13, 2026

Copy link
Copy Markdown

Claude finished @MauriceVanVeen's task in 3m 46s —— View job


PR Review

  • Gather context and read CLAUDE.md
  • Read PR diff and changed files
  • Analyze changes for correctness, concurrency safety, and performance
  • Post inline review comments
  • Post review summary

Verdict: LGTM — correct fix, well-tested.

The PR correctly addresses three distinct paths where n.pae could survive past the entries it was caching:

truncateWAL: Selective delete (index > truncation point) is the right approach — it preserves still-valid cache entries rather than wiping everything. Lock ordering is safe: the WAL is truncated first, then the cache is invalidated, all under the node lock, so applyCommit cannot observe a split state.

installSnapshot (direct call): The condition snap.lastIndex > n.commit is correct — entries ≤ n.commit have already been evicted from n.pae by applyCommit's defer delete(...). Only uncommitted entries at indices (n.commit, snap.lastIndex] are stale after WAL compaction.

Catchup snapshot reordering: This is the subtle and crucial fix. Before, n.commit = ae.pindex was set before calling installSnapshot, making the clearing condition inside trivially false (snap.lastIndex == n.commit). The reordering ensures the old n.commit is compared against the snapshot index, correctly triggering the cache flush. Bonus: if installSnapshot errors, the node fields are now left unchanged rather than in a partially-mutated state.

Tests: All four test functions clearly exercise the intended scenario, use the existing single-node test harness, and check both the positive (correct state after) and negative (cache is gone) conditions.

Minor observations (neither blocking nor new to this PR):

  • When n.pae = make(map[uint64]*appendEntry) is called, the abandoned *appendEntry pool objects are not returned to aePool. This is consistent with the pre-existing pattern in switchToLeader (line 5131) — GC will collect them, so no true leak, but pool reuse efficiency is reduced.
  • The full-wipe in installSnapshot also drops any entries at indices > snap.lastIndex that would still be valid in the WAL. Acceptable for a recovery path; and in practice the caller immediately sets n.pindex = ae.pindex, so there are no such future-indexed entries anyway.

Comment thread server/raft.go
Comment thread server/raft.go
Comment thread server/raft.go

@sciascid sciascid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@neilalexander
neilalexander merged commit d77c831 into main May 13, 2026
58 checks passed
@neilalexander
neilalexander deleted the maurice/truncate-pae branch May 13, 2026 14:26
neilalexander added a commit that referenced this pull request May 14, 2026
neilalexander added a commit that referenced this pull request May 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants