storage: sstable missing from EAR registry

[jbowens]

Internal discussion
https://cockroachlabs.slack.com/archives/C057ULDSKC0/p1689098898636449?thread_ts=1688047254.186649&cid=C057ULDSKC0

Jira issue: CRDB-29644

Epic: CRDB-26603

[blathers-crl]

Hi @jbowens, 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 dev-inf.}

[itsbilal]

Taking a look. From code-reading, nothing has particularly stood out to me as an obvious bug in the file registry code or so, around deletions or additions of files.

The missing sstable 67105381 is not mentioned in the latest file registry at all, and I don't see it in a dump of each individual record either - so it must have been added/removed either before the last rotation of the file registry, or was never added at all. I used a modified build of Cockroach to decrypt the manifest without having the rest of the store, and I can see the file being added to the manifest in the second newest manifest. And the file is still linked in the newest Version, so it shouldn't be deleted yet.

The remaining questions:

1. Did this store always have EAR, or was EAR enabled somewhat recently?
2. Are there any other things to be aware of about this store (eg. fsync disabled, unique file system)?

[sumeerbhola]

We have a bug in the file registry which dates from the start of the record writer based registry in 239377a that can lose entries.

The writeToRegistryFile first writes the new batch, and then if the file is too big, creates a new registry file

cockroach/pkg/storage/pebble_file_registry.go

Lines 426 to 430 in 30acaf9

	if r.mu.registryWriter.Size() > maxRegistrySize {
	if err := r.createNewRegistryFile(); err != nil {
	return errors.Wrap(err, "rotating registry file")
	}
	}

. The new registry file is initialized with the map. But the map has not yet been updated to include the effect of the batch since that update happens after writeToRegistryFile returns, by calling applyBatch

cockroach/pkg/storage/pebble_file_registry.go

Lines 379 to 382 in 30acaf9

	if err := r.writeToRegistryFile(batch); err != nil {
	panic(err)
	}
	r.applyBatch(batch)

.

So the update provided by the batch gets lost (it is not recorded persistently).
We probably did not discover this earlier if most lost updates were deletes, or when there were new files, those files happened to be GC'd due to compactions before the CockroachDB node was restarted.

[itsbilal]

Oh wow, nice catch @sumeerbhola. I read that code multiple times over and didn't catch the bug.

[sumeerbhola]

I read that code multiple times over and didn't catch the bug.

I didn't either. A new test caught it.

[jbowens]

Summary: When the encryption-at-rest registry rolls over (at 128 MiB), the entry that triggered the rollover is lost. This can result in the loss of persistence of encryption metadata for up to 1 file whose creation triggered the rollover. If the file is not deleted (eg, compacted or rotated out) before the node's next restart, the restarted process considers the file to be corrupt, resulting in the loss of the store.

This bug has existed since 21.2 but we believe we've observed at least three occurrences in the past three months: this issue in a roachprod cluster, support#2340, and the CC incident on 2023-07-28.