apply: fix initial snapshot assertion

[pav-kv]

The broad assertion here is that all the entries composing a snapshot must have been applied by someone before (at least the sender of the snapshot has done it, otherwise it couldn’t have sent this snapshot).

And then there is an additional assertion (which failed occasionally before this PR) that there can’t be a snapshot without at least one entry applied. This is generally true, except that there are entries not registered by the "apply" stack: particularly, empty entries that raft leader appends at the start of its term. There is a comment to this extent in the asserter.go code:

cockroach/pkg/kv/kvserver/apply/asserter.go

Lines 254 to 259 in 5b0371b

	// INVARIANT: all commands have a command ID. etcd/raft may commit noop
	// proposals on leader changes that do not have a command ID, but we skip
	// these during application.
	if len(cmdID) == 0 {
	fail("applied command without ID")
	}

This commit modifies the snapshot assertion to correctly handle this case. It is possible that an initial snapshot sent from leader to followers contains only this dummy entry, and there were no "real" proposals applied.

Fixes the false assertion failure in #118471 (comment)
Related to #116319

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

[cockroach-teamcity]

This change is 

[arulajmani]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @pav-kv and @tbg)

---

-- commits line 3 at r1:
This commit message deserves some words about the previous assumption, why it was faulty, and the fix. You already had this in Slack from earlier today, but it's worth capturing here as well.

---

pkg/kv/kvserver/apply/asserter.go line 392 at r1 (raw file):

	// is an initial snapshot. It's possible that the initial snapshot follows an
	// empty entry appended by the raft leader at the start of this term. Since we
	// don't register this entry as applied, r.log can be empty here.

nit: Consider adding more words to this explanation and pointing to

cockroach/pkg/kv/kvserver/apply/asserter.go

Lines 254 to 259 in 0ea0320

	// INVARIANT: all commands have a command ID. etcd/raft may commit noop
	// proposals on leader changes that do not have a command ID, but we skip
	// these during application.
	if len(cmdID) == 0 {
	fail("applied command without ID")
	}

here.

[pav-kv]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani and @tbg)

---

-- commits line 3 at r1:

Previously, arulajmani (Arul Ajmani) wrote…

This commit message deserves some words about the previous assumption, why it was faulty, and the fix. You already had this in Slack from earlier today, but it's worth capturing here as well.

Done

---

pkg/kv/kvserver/apply/asserter.go line 392 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: Consider adding more words to this explanation and pointing to

cockroach/pkg/kv/kvserver/apply/asserter.go

Lines 254 to 259 in 0ea0320

	// INVARIANT: all commands have a command ID. etcd/raft may commit noop
	// proposals on leader changes that do not have a command ID, but we skip
	// these during application.
	if len(cmdID) == 0 {
	fail("applied command without ID")
	}

here.

Done

[pav-kv]

bors r=arulajmani

[craig]

Build succeeded:

• linux_arm64_build
• macos_amd64_build
• acceptance
• local_roachtest_fips
• local_roachtest
• examples_orms
• windows_build
• linux_amd64_fips_build
• linux_amd64_build
• lint
• unit_tests
• docker_image_amd64
• check_generated_code
• macos_arm64_build