fix: synchronize totalChecks increment in NostrEventDeduplicator

isDuplicate() incremented totalChecks outside the lruLock that guards
every other mutation on this class, so concurrent callers raced on the
read-modify-write and lost increments. duplicateCount and evictionCount
were already incremented under the lock; totalChecks was the one
counter left outside it.

getStats().totalChecks feeds hitRate and is the only denominator for
duplicateCount, so a systematic undercount skews both. Moved the
increment inside the existing synchronized block.

Added a concurrency test that reproduces the race: run against the
prior code it failed reliably (3/3 runs); against the fix it passes.
This commit is contained in:
Taksh 2026-08-10 20:09:00 +05:30
parent 55fd6ad9bd
commit ac47af9101
2 changed files with 68 additions and 2 deletions

View File

@ -79,9 +79,9 @@ class NostrEventDeduplicator(
* @return true if the event is a duplicate (already seen), false if it's new
*/
fun isDuplicate(eventId: String): Boolean {
totalChecks++
synchronized(lruLock) {
totalChecks++
val existingNode = nodeMap[eventId]
if (existingNode != null) {

View File

@ -0,0 +1,66 @@
package com.bitchat.android.nostr
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import java.util.concurrent.CountDownLatch
import java.util.concurrent.Executors
import java.util.concurrent.TimeUnit
class NostrEventDeduplicatorTest {
@Test
fun `isDuplicate flags a repeated event id`() {
val deduplicator = NostrEventDeduplicator(maxCapacity = 10)
assertEquals(false, deduplicator.isDuplicate("event-1"))
assertEquals(true, deduplicator.isDuplicate("event-1"))
}
@Test
fun `capacity evicts the least recently used event id`() {
val deduplicator = NostrEventDeduplicator(maxCapacity = 2)
deduplicator.isDuplicate("a")
deduplicator.isDuplicate("b")
deduplicator.isDuplicate("c") // evicts "a"
assertTrue(deduplicator.contains("b"))
assertTrue(deduplicator.contains("c"))
assertEquals(false, deduplicator.contains("a"))
}
/**
* totalChecks used to be incremented outside the lruLock that guards every other
* mutation in this class, so concurrent callers could race on the read-modify-write
* and lose increments. Every other counter (duplicateCount, evictionCount) was
* already incremented under the lock, which is why only this one drifted.
*/
@Test
fun `totalChecks counts every call exactly once under concurrent access`() {
val deduplicator = NostrEventDeduplicator(maxCapacity = 10_000)
val threadCount = 8
val checksPerThread = 2_000
val executor = Executors.newFixedThreadPool(threadCount)
val start = CountDownLatch(1)
val done = CountDownLatch(threadCount)
repeat(threadCount) { threadIndex ->
executor.submit {
start.await()
repeat(checksPerThread) { callIndex ->
deduplicator.isDuplicate("thread-$threadIndex-event-$callIndex")
}
done.countDown()
}
}
start.countDown()
assertTrue(done.await(10, TimeUnit.SECONDS))
executor.shutdown()
assertEquals(
(threadCount * checksPerThread).toLong(),
deduplicator.getStats().totalChecks
)
}
}