Panic due to NPE when processing WiredList

[justinjc]

Stack trace:

panic: runtime error: invalid memory address or nil pointer dereference

github.com/m3db/m3/src/x/ident.BytesID.Equal(...)
    github.com/m3db/m3/src/x/ident/bytes_id.go:46
github.com/m3db/m3/src/dbnode/storage/series.(*dbSeries).OnEvictedFromWiredList(...)
    github.com/m3db/m3/src/dbnode/storage/series/series.go:580
github.com/m3db/m3/src/dbnode/storage.(*dbShard).OnEvictedFromWiredList(...)
    github.com/m3db/m3/src/dbnode/storage/shard.go:532
github.com/m3db/m3/src/dbnode/storage/block.(*WiredList).insertAfter(...)
    github.com/m3db/m3/src/dbnode/storage/block/wired_list.go:299
github.com/m3db/m3/src/dbnode/storage/block.(*WiredList).pushBack(...)
    github.com/m3db/m3/src/dbnode/storage/block/wired_list.go:348
github.com/m3db/m3/src/dbnode/storage/block.(*WiredList).processUpdateBlock(...)
    github.com/m3db/m3/src/dbnode/storage/block/wired_list.go:239
github.com/m3db/m3/src/dbnode/storage/block.(*WiredList).Start.func1(...)
    github.com/m3db/m3/src/dbnode/storage/block/wired_list.go:169

This is a regression after an upgrade from:
b429452 (from v0.14.2)
to:
99477fc (from v0.15.12)
Update: This bug appears to have been present for a very long time.

The stack trace indicates that the id in the series is nil. Judging purely from the comment block above the id field:

m3/src/dbnode/storage/series/series.go

Lines 64 to 68 in 99477fc

	// Note: The bytes that back "id ident.ID" are the same bytes
	// that are behind the ID in "metadata doc.Document", the whole
	// reason we keep an ident.ID on the series is since there's a lot
	// of existing callsites that require the ID as an ident.ID.
	id ident.ID

I'm guessing that this might be related to this commit: b4be7b1

cc: @robskillington

[robskillington]

Hm @justinjc I think this actually might be a race case where a series is evicted from the wired list and it is also evicted from the shard map and therefore s.id is nil on the series.

Could you perhaps try adding a closed bool to the series and checking that at the beginning of "OnEvictedFromWiredList" before checking against the ID?

If that works we may want to create this concept of open or closed, and then set it to false on "Reset(...)" and then set it true on "Close()".

This will help us discern whether it's a valid use of the series or if it's simply not in an open state and used outside of the shard lock map (like the case here where the series is being used outside of the RLock/RUnlock being held of the shard map).

[justinjc]

Quick update: it appears that this bug is present in older versions too (pre-v0.15.x). We seemed to have encountered this in clusters with old builds, but for some reason it procs significantly more frequently with new builds.

I've attempted to repro this with this test, which continuously writes and flushes series so that the series gets evicted (closed), nil-ing the ID. At the same time, I fetch IDs in a loop to continuously evict series from the wired list. The test does repro this issue, albeit very inconsistently.