fix(pvtdatastorage): close collItr before releasing purgerLock in processCollElgEvents

[veyron-kairo]

I found a bug in processCollElgEvents where a stale LevelDB iterator can cause expired private data to be silently re-added to the store, making the reconciler loop forever trying to fetch data that no longer exists.

What's happening

When converting ineligible missing data entries to eligible ones, the function batches up writes and sleeps between batches to avoid hammering the DB. Before sleeping it drops purgerLock:

s.db.WriteBatch(batch, true)
s.purgerLock.Unlock()
time.Sleep(sleepTime * time.Millisecond)
s.purgerLock.Lock()

The problem is that collItr — a LevelDB snapshot iterator over the ineligible missing data range — is still open when the lock is released. LevelDB snapshot iterators capture the DB state at the moment they're created and keep returning those keys regardless of what happens to the DB afterwards.

So while we're sleeping, the purger goroutine wakes up (it was blocked waiting for purgerLock), runs purgeExpiredData, and deletes both the eligible and ineligible missing data entries for any BTL-expired collections. When we re-acquire the lock and keep iterating, the stale snapshot still hands us those deleted keys, and we write them back to the DB as eligible missing data:

batch.Delete(originalKey)                                    // already gone, no-op
batch.Put(encodeElgPrioMissingDataKey(modifiedKey), copyVal) // re-inserts expired entry

The reconciler then picks these up, asks every peer for the data, and every peer says no — because it's been purged everywhere. This repeats until the purger happens to run again and cleans up the re-inserted entries.

The fix

Close collItr before releasing the lock. After re-acquiring the lock, reopen a fresh iterator starting from the last-processed key. Since that key was already deleted by the batch we just flushed, the new iterator skips it and starts from the next real entry — and this time it reflects the actual DB state, including whatever the purger deleted during the sleep.

nextKey := make([]byte, len(originalKey))
copy(nextKey, originalKey)
collItr.Release()
s.purgerLock.Unlock()
time.Sleep(sleepTime * time.Millisecond)
s.purgerLock.Lock()
collItr, err = s.db.GetIterator(nextKey, endKey)
if err != nil {
    return err
}

Why it's hard to notice

There's no crash and the peer keeps running normally. The only signal is the reconciler logging failures to fetch private data, which happens in normal operation too (e.g. when peers are temporarily unreachable). You'd have to specifically correlate those warnings with collection eligibility events and purge intervals to suspect this. In practice it just looks like slow or noisy reconciliation.

[veyron-kairo]

@pfi79 , please let me know your thoughts on this !

[pfi79]

@pfi79 , please let me know your thoughts on this !

A magnificent find. Thank you for your hard work.
Is it possible to add a test for this case that would fail without your correction?

[veyron-kairo]

Thanks @pfi79! I've added a regression test (TestCollElgEnabled_PurgerDeletesDuringBatchSleep) that fails without the fix and passes with it.

It works by setting MaxBatchSize=1 so every entry triggers a batch flush. During the inter-batch sleep (when purgerLock is released), a goroutine grabs the lock and deletes the remaining InelgMissingData keys — mimicking what the real purger would do. Without the fix, the stale iterator still sees those deleted keys and re-inserts them as ElgPrioMissingData. With the fix, the reopened iterator picks up the deletions correctly.

[veyron-kairo]

Hey @pfi79, the failing Integration Tests (raft) seems unrelated to this change — it's a TLS certificate rotation test (is still possible to onboard new orderers) hitting a quorum loss error. I noticed the same raft test is also failing on another open PR (bump-grpc-1.79.3), so it looks like a flaky test at the moment.

All pvtdatastorage unit tests pass, including the new regression test. Could you re-run the failed job when you get a chance? Thanks!