storage: pebbleIterator panics too eagerly on Close()

[msbutler]

After hooking the new pebbleIterator into the restore processor (#83984), a few restore roachtests on master have been failing (#84240, #84237), because the pebbleIterator panics when it attempts to close, for example:

panic: EOF                                                                      
    9                                                                                 
   10 goroutine 45749 [running]:                                                      
   11 panic({0x483a900, 0xc0000a4120})                                                
   12         GOROOT/src/runtime/panic.go:1147 +0x3a8 fp=0xc0061cb528 sp=0xc0061cb468 pc=0x48a828
   13 github.com/cockroachdb/cockroach/pkg/storage.(*pebbleIterator).destroy(0xc0039f0000)
   14         github.com/cockroachdb/cockroach/pkg/storage/pebble_iterator.go:897 +0x1e5 fp=0xc0061cb668 sp=0xc0061cb528 pc=0x17a61a5
   15 github.com/cockroachdb/cockroach/pkg/storage.(*pebbleIterator).Close(0xc0039f0000)

(see full example panic stack trace in comment).

In the failed tests and in repros, the panic reasons have been EOF, Context cancelled, and nil, which at a high level, makes me wonder if all this panicking is necessary. To get a better understanding of when the iterator panics, I ran the restore/nodeShutdown/coordinator roachtest on my gce-worker using master + a commit that adds more logging related to the SSTs in the iterator and issues prints instead of panics in pebbleIterator.destroy().

I can explain and reproduce the context cancelled panic. When the restore fails for some independent reason, context cancellations circulates throughout the goroutines, leading to pebbleIterator.Valid() returning !ok and a context cancelled error. This then leads to defer.Close(), surfacing the context cancelled error during destroy(), leading to a panic. To repro this, stress the restore/nodeShutdown/coordinator roachtest above, potentially with my augmented commit.

I'm not sure what causes the EOF or nil based panics.
@erikgrinaker

Jira issue: CRDB-17633

Epic CRDB-2624

[blathers-crl]

Hi @msbutler, please add branch-* labels to identify which branch(es) this release-blocker affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[erikgrinaker]

Looking at the Pebble code, it seems like it might return the previous iterator error when calling Close():

https://github.com/cockroachdb/vendored/blob/01829831ec6297da9911a4ac4bff34c3a7d97561/github.com/cockroachdb/pebble/iterator.go#L1720-L1743

So if we're seeing an error during iteration, then Close() will also return that error, but a close error always panics in pebbleIterator here:

cockroach/pkg/storage/pebble_iterator.go

Lines 895 to 898 in c58c0aa

	err := p.iter.Close()
	if err != nil {
	panic(err)
	}

@jbowens Is this new behavior? It seems problematic, because any iteration error will essentially cause a CRDB panic when used together with defer iter.Close().

[jbowens]

@erikgrinaker — no, I'm pretty sure that's the way it's always been:

https://github.com/cockroachdb/cockroach/blame/c58c0aac82480c069d71717bac1df317798317fd/pkg/storage/pebble_iterator.go#L897
https://github.com/cockroachdb/pebble/blob/crl-release-22.1/iterator.go#L1492

It does seem problematic that an ephemeral I/O iteration error causes a CockroachDB panic, although ephemeral I/O errors also cause a panic in some compaction code paths (eg, cockroachdb/pebble#1794).

There are times where pebble iterators will surface an error due to misuse (eg, SeekPrefixGE bounds checks), in which a panic is appropriate. Also, as long as we don't have graceful corruption recovery (#67568), panicking on sstable checksum errors surfaced by iteration is also appropriate.

We can choose to not panic, or panic only on errors not known to be ephemeral, but what do we do with the ephemeral error? The current storage package interface doesn't provide a means to differentiate between an iterator exhausted due to an error, versus an iterator exhausted successfully. If we don't panic, it's not sufficient to just defer iter.Close(), because iteration may have ended prematurely due to an error.

I'm assuming we're running into this only now because only now we're using a vfs.File implementation that's backed by the network?

[jbowens]

Hmm, I forgot that MVCCIterator.Valid() has an error return that uses i.iter.Error. I don't see why we can't rely on exclusively surfacing iteration errors through that.

[jbowens]

the panic reasons have been EOF,

Is it possible we're wrapping io.EOF somewhere in the vfs.File implementation? Like the io.EOF godoc references, Pebble does perform an equality check comparison to io.EOF when reading the tail of a sstable. Wrapping could cause io.EOF to be treated as an error.

EOF is the error returned by Read when no more input is available. (Read must return EOF itself, not an error wrapping EOF, because callers will test for EOF using ==.) Functions should return EOF only to signal a graceful end of input. If the EOF occurs unexpectedly in a structured data stream, the appropriate error is either ErrUnexpectedEOF or some other error giving more detail.

where's the networked implementation of vfs.File live?

[msbutler]

In the PR I reverted, restore called ExternalSSTReader() to instantiate a PebbleSSTIterator, see here.

The basic logic is:

• The caller passes an externalStorage interface, which ExternalSSTReader() uses to call ReadFileAt, returning a file handler ( ioctx.ReadCloserCtx). Here's is our GCS implementation of ReadFileAt.
• ExternalSSTReader() then binds the file handler to our sstReader struct, which implements sstable.ReadableFile, a subset of the vfs.file interface.

https://github.com/msbutler/cockroach/blob/4d1f66e12d174fb3bf59549c4f660931552ffc8d/pkg/ccl/storageccl/external_sst_reader.go#L122-L130

• This readableFile is passed to the PebbleSSTIterator

Perhaps the new PebbleIterator doesn't play nice with the sstReader? It worked fine with storage.NewSSTIterator