Cursor.Delete followed by Next skips the next k/v pair

[jrick]

After fighting an inexplicable bug in a package that makes heavy use of bolt behind one of our own interfaces, I've finally nailed down the issue to what appears to be a bug with bolt's cursors, where the next k/v pair is always skipped when iterating over keys, and while each key is deleted during the iteration. Based on my understanding of the bolt documentation, deleting items using the cursor is the only modification that can be performed to the database during cursor iteration without invalidating the cursor.

I added a test, which fails, to my current project that exposes the issue (sorry, it's late and I've been fighting this issue all day, and to top it all off GitHub has lost this issue twice now as I was writing it up, so I don't yet have an isolated test that doesn't consume any of our own code):

https://github.com/jrick/btcwallet/blob/95ae8a42a9b8b2d70a9ab2289f3f05511cf27bf3/wtxmgr/db_test.go#L80

=== RUN   TestBoltDBCursorDeletion
--- FAIL: TestBoltDBCursorDeletion (0.01s)
	db_test.go:137: Expected to iterate over and delete four k/v pairs, but iterated 2 time(s)
	db_test.go:142: bucket still has key 1812fefbbfe53afef21762bcb138d77e600416541e0235c2554435063f21292b00000001 value 000000000bebc2001e82000000000000000000000000
	db_test.go:142: bucket still has key 1812fefbbfe53afef21762bcb138d77e600416541e0235c2554435063f21292b00000003 value 0000000017d784001e82000000000000000000000000

As you can see, it successfully removed the first and third keys (ending in 00000000 and 00000002, but left the two keys ending in 00000001 and 00000003).

We use glide to maintain our dependencies and currently have bolt locked to commit 583e8937c61f1af6513608ccc75c97b6abdf4ff9 (version 1.3.0).

[smitajit]

After c.Delete() the cursor automatically moves to next item. after that c.Next() moves to further one item.

Changing to c.First() should fix your testcase.

c = b.Cursor()
		for k, _ := c.Seek(txHash[:]); bytes.HasPrefix(k, txHash[:]); k, _ = c.First() {
			err = c.Delete()
			if err != nil {
				return err
			}
			iterations++
}

[jrick]

Sorry but that API is borderline unusable if that is how it is intended to work. We need to inspect the k/v to determine whether to delete it before moving onto the next item, and there is no way to access that next k/v after deleting item pointed to by the cursor. Resetting the cursor back to the beginning only works when all items are being deleted.

[justinfx]

Is this still the way the API works? I have a similar situation, where I am iterating on a cursor and conditionally deleting items. Do we need to account for the position of the cursor just after we delete, such as calling Prev() to allow the for-loop to advance as normal?

[chuckg]

Be really hard to tell if the bug is in your iterator, or in BoltDB. I have this exact use case implemented, but I use Bucket.Delete() while the iterator walks. In this way, it doesn't change my iterators position as I conditionally walk the bucket and delete. I have tested and validated the pattern. Use your transaction to grab the bucket it's working on, then drop rows.

[jrick]

@chuckg the bug isn't in my iterator wrapper apis but in bolt itself. The linked test is testing the bolt apis without using any of our projects db wrappers over it.

Modifying the bucket while it is being iterated over, except through Cursor.Delete (and Cursor.Delete is buggy), invalidates the cursor and you may receive unexpected results.

[chuckg]

From the README, the only mention of deletes is Use the Bucket.Delete() function to delete a key from the bucket. I suspect that calling Bucket.Delete() spins up a new Cursor and as long as I don't try to walk back into the data I've just deleted, my current cursor remains valid.

The reason I said it might be your API is that I don't see you using basic bolt functions in your test:

ns := dbtx.ReadWriteBucket(wtxmgrNamespaceKey)
....
....
// Test the iterator iterates over each.
it := makeUnminedCreditIterator(ns, txHash)

Those don't seem like current Bolt primitives. In the interest of giving the Bolt authors some fodder to work with, can you refactor your test to only use the bolt API?

[jrick]

The function i linked directly to (TestBoltDBCursorDeletion) only calls bolt APIs on the bolt database. It is using some of my project's functions for keys and values but that is unrelated.

[logrusorgru]

This should probably help

for k, _ := c.Seek(txHash[:]); bytes.HasPrefix(k, txHash[:]); k, _ = c.Seek(txHash[:]) {
	err = c.Delete()
	if err != nil {
		return err
	}
	iterations++
}