Skip to content

fix(hnsw): refresh dead-node set instead of caching it for the index lifetime#9755

Open
shaunpatterson wants to merge 1 commit into
dgraph-io:mainfrom
shaunpatterson:sp/fix-hnsw-stale-deadnodes
Open

fix(hnsw): refresh dead-node set instead of caching it for the index lifetime#9755
shaunpatterson wants to merge 1 commit into
dgraph-io:mainfrom
shaunpatterson:sp/fix-hnsw-stale-deadnodes

Conversation

@shaunpatterson

@shaunpatterson shaunpatterson commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Problem

(*persistentHNSW).removeDeadNodes loaded the tombstoned-vector set (the persisted __vector_dead posting) into ph.deadNodes once, on the first call, and never refreshed it:

// TODO add a path to delete deadNodes
if ph.deadNodes == nil { /* load from vecDead posting, build the map */ }

The index instance lives in the factory's indexMap for the life of the process, so once populated the set was frozen. Any vector deleted after the first removeDeadNodes call was never seen by the neighbour filter, and dead UIDs leaked back into edge lists during subsequent inserts and neighbour updates. The standing // TODO add a path to delete deadNodes acknowledged the gap.

Fix

Cache the dead set as an immutable snapshot tagged with the transaction read timestamp (TxnCache.Ts()), published via atomic.Pointer[deadSnapshot]:

  • Correctness — a transaction with a newer StartTs reloads the set and observes deletions committed since the previous load.
  • Snapshot isolation — the shared cache only ever advances in time. If a newer snapshot is already cached, an older-ts caller is served its own ts-scoped set and does not install it, so a transaction never observes deletions newer than its own snapshot.
  • Performance — an index rebuild streams every key at one StartTs, so the JSON is parsed once and reused across the many removeDeadNodes calls per insert.
  • Concurrency — the index instance is shared across the goroutines that drive a rebuild. Publication is lock-free via atomic.Pointer + CompareAndSwap; the snapshot map is immutable after construction, so the filter reads it without synchronization.

The hot path (removeDeadNodes) delegates to loadDeadNodes(tc), which returns the cached snapshot on a timestamp match and otherwise loads, builds, and CAS-publishes a new one.

Test

  • TestRemoveDeadNodesRefreshesAcrossTimestamps — cross-timestamp refresh + within-snapshot stability
  • TestRemoveDeadNodesSnapshotIsolation — an older-ts caller is unaffected by a newer cached snapshot
  • TestLoadDeadNodesConcurrent — concurrent mixed-timestamp loads under -race

go test -race ./tok/..., go vet, gofmt all clean; golangci-lint (repo config) reports no new findings from this change.

🤖 Generated with Claude Code

@shaunpatterson shaunpatterson requested a review from a team as a code owner June 19, 2026 11:47
@shaunpatterson shaunpatterson force-pushed the sp/fix-hnsw-stale-deadnodes branch from c5a3212 to a8e9924 Compare June 19, 2026 12:26
…lifetime

removeDeadNodes loaded the tombstoned-vector set (the persisted vecDead
posting) into ph.deadNodes exactly once, on the first call, and never
refreshed it (the `if ph.deadNodes == nil` guard, with a standing
`// TODO add a path to delete deadNodes`). Any vector deleted after that
first call stayed invisible to the neighbour filter for the lifetime of the
index instance, so dead UIDs leaked back into edge lists during subsequent
inserts and neighbour updates.

Cache the set as an immutable snapshot tagged with the transaction read
timestamp, published via an atomic.Pointer:

- Correctness: a transaction with a newer StartTs reloads and observes
  deletions committed since the previous load.
- Snapshot isolation: the shared cache only ever advances in time. If a
  newer snapshot is already cached, an older-ts caller is served its own
  ts-scoped set and does not install it, so it never observes deletions
  newer than its own snapshot.
- Performance: a rebuild streams every key at one StartTs, so the JSON is
  parsed once and reused across the many removeDeadNodes calls per insert.
- Concurrency: the index instance is shared across the goroutines that drive
  a rebuild. Publication is lock-free via atomic.Pointer + CompareAndSwap;
  the snapshot map is immutable after construction, so the filter reads it
  without synchronization.

Tests cover the cross-timestamp refresh, within-snapshot stability,
snapshot-isolation (older caller unaffected by a newer cached snapshot), and
concurrent mixed-timestamp loads under -race.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@shaunpatterson shaunpatterson force-pushed the sp/fix-hnsw-stale-deadnodes branch from a8e9924 to 98a6a65 Compare June 19, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant