Carry the installed snapshot index in the install-snapshot acknowledgement

[tiandiwonder]

Problem

A leader silently discards a follower's successful snapshot-install acknowledgement whenever the install takes longer than snapshot_sync_ctx_timeout, and then has no record that the follower holds the snapshot at all. The follower is fully caught up and says so; the leader does not believe it.

This is not a rare race. The timeout is a per-round-trip responsiveness budget — its own doc comment says "if a single snapshot syncing request exceeds this", and snapshot_sync_ctx::set_offset resets the timer on every offset advance — and it defaults to raft_limits_response_limit * heart_beat_interval_. But the final round trip of a logical-object install contains the state machine's apply_snapshot, which is indivisible, so no snapshot_transfer_chunk_size can bring that round trip under a responsiveness budget. Measured on a production ClickHouse Keeper ensemble against a 10 s derived budget:

snapshot	chunks	transfer	apply	timed out
254.8 MB	1	2 s	14 s	yes, 10002 ms
400.9 MB	1	3 s	17 s	yes, 10000 ms
418.6 MB	4	11 s	19 s	yes, 10851 ms

The third row is the load-bearing one: chunking was active and bounded each transfer round trip to a few seconds, and the timeout still fired inside the 19 s apply.

Concrete consequences, all observed on that ensemble:

• The leader logs snapshot install task for peer N timed out: 10000 ms, reset snapshot sync context, immediately followed by no snapshot sync context for this peer, drop the response — even though the follower logged successfully receive a snapshot and applied it.
• next_log_idx, matched_idx and in particular next_log_idx_floor are never set from the installed snapshot, so nothing bounds a subsequent backward log walk. On builds without Fix append sync below snapshot boundary #109 the leader then rewinds one index per round trip, logging declined append: peer N, prev next log idx ..., resp next ... repeatedly. On builds with Fix append sync below snapshot boundary #109 the walk is absorbed in the same second via peer N compacted snapshot boundary, but the wasted install still happens.
• The follower stayed out of quorum for 17.5 minutes, twice within one hour, producing roughly 10.8M client Session expired errors.
• Because matched_idx is never set, the peer does not count toward the commit quorum for that whole interval.
• If the leader has since produced a newer snapshot, the peer is sent a second full install of it. The timeout does not clear snapshot_sync_is_needed, and a follower answering RECEIVING_SNAPSHOT while applying is exactly what sets that flag.

Root cause

raft_server::handle_hb_timeout → request_append_entries calls check_snapshot_timeout (src/handle_snapshot_sync.cxx), which on expiry calls clear_snapshot_sync_ctx and destroys the peer's snapshot_sync_ctx. When the follower's terminal response finally arrives, handle_peer_resp dispatches it to raft_server::handle_install_snapshot_resp, which finds p->get_snapshot_sync_ctx() == nullptr and returns after logging the drop — so the three writes on the success path, all of which read sync_ctx->get_snapshot()->get_last_log_idx(), never run.

The drop itself is correct given the information available, which is why it has survived: resp_msg carries term/src/next_idx/accepted/ctx and not the installed snapshot's Raft index. For logical_object snapshots — what ClickHouse Keeper sends via KeeperStateMachine::read_logical_snp_obj — next_idx is an object cursor and the terminal ctx is a bare done flag: a single zero byte written in handle_install_snapshot_req. The index exists only inside sync_ctx. Without it the response is genuinely uninterpretable, and binding a peer to the wrong snapshot on unverifiable information would be worse than dropping. So the defect is not the timeout and not the drop — it is that the acknowledgement is not self-describing.

Fix

Make the acknowledgement carry the one thing the leader's completion bookkeeping actually needs from the context: the installed snapshot's last_log_idx. The new snp_install_done_ctx in src/handle_snapshot_sync.cxx owns the format, the follower emits it at the single genuinely successful terminal site in handle_install_snapshot_req, and the leader reads it in handle_install_snapshot_resp:

Format tag                       1 byte
Installed snapshot last_log_idx  8 bytes (little endian)

len == 1 with tag 0 remains the historical indexless marker. For every tag >= 1, bytes 1..8 are the index — fixing that prefix lets a leader read the index out of a tag it does not otherwise understand, so later extensions stay additive. Anything else is malformed and is rejected rather than read as a completion: falling back to "treat it as the old marker" would be the permissive reading here, because in this code the old marker means completion.

All parsing happens inside the resp.get_accepted() path and nowhere else, so rejected responses keep their current behaviour verbatim — that branch deliberately rewrites the peer's position and zeroes the floor as follower-ahead recovery, which must not be disturbed. Within the accepted path:

• With no sync context, an acknowledgement carrying an index is acted upon instead of dropped, provided the index passes validation: the response term is current, the index lies in (0, precommit_index_], and idx + 1 cannot wrap. Every write is monotone (max), so a late acknowledgement can never move a peer backwards — that is what makes acting without the context safe at all. The rest of the success bookkeeping runs too, including clearing snapshot_sync_is_needed; without that last part the fix would save little, since create_append_entries_req would start another full install as soon as the leader held a newer snapshot.
• With a sync context whose snapshot has a different index, the in-flight install is no longer treated as done. matched_idx feeds get_expected_committed_log_idx, so crediting a peer with a snapshot it never installed is a safety problem and not merely bookkeeping. Only the index that really was installed is credited, monotonically, and the live transfer is left to finish and report for itself.

The timeout is not removed and the drop is not made unconditional. Some timeout must exist, because clear_snapshot_sync_ctx → destroy_user_snp_ctx is what releases the state machine's user snapshot context; holding that open indefinitely for a possibly-dead follower would leak. This change only makes the drop a fallback instead of the normal outcome.

Eight cases were added to tests/unit/snapshot_test.cxx: late_snapshot_install_ack_accepted_test, ..._monotonic_test, ..._no_reinstall_test, ..._validation_test, ..._wrong_snapshot_test, ..._legacy_marker_test, ..._malformed_test and ..._future_format_test. Five fail without this change (accepted, no-reinstall, wrong-snapshot, malformed, future-format), verified by reverting src/handle_snapshot_sync.cxx while keeping the tests; the other three pass on the unpatched code because it drops the acknowledgement or is unchanged by design, so they guard the new logic rather than reproduce the defect. The pre-existing suites are what guarantee the untouched paths still hold — in particular snapshot_basic_test, snapshot_new_member_restart_test and snapshot_leader_switch_test cover normal, timely installs, and snapshot_rewind_floor_test covers the #109 floor clamping that this change now feeds a non-zero floor into. All pass with cmake -DDISABLE_ASIO=1: snapshot_test 33/33, raft_server_test 26/26, leader_election_test 9/9, learner_new_joiner_test 4/4, failure_test 7/7, serialization_test 11/11, buffer_test 5/5. The change was also built and run inside ClickHouse — 177/177 Keeper and coordination unit tests.

raft_server_handler and FakeNetwork gain accessors so the tests drive the real check_snapshot_timeout and read precommit_index_ rather than emulating the post-timeout state.

Compatibility

Degrades safely in both directions, which matters because the incident happened during a mixed-version rollout:

direction	behaviour
new follower → old leader	an old leader's completion test for logical-object snapshots is only that resp_msg::get_ctx is non-null, and it never inspects the payload, so a 9-byte context is still read as done
old follower → new leader	a 1-byte tag-0 context takes the legacy row above and behaves exactly as before

Deliberately not addressed

• Legacy acknowledgements carry no index, so an old follower plus a live sync context still cannot be correlated. That needs a different mechanism — an echoed request id, or a generation counter on the sync context.
• raw_binary snapshots, whose completion test is resp.get_next_idx() >= snp->size() and needs the snapshot object regardless.
• handle_install_snapshot_resp_new_member, the add-server path, which has the identical shape. The follower emits the extended payload there too, which is harmless because that handler ignores the payload.

Question for reviewers

Is extending the terminal ctx acceptable, or would you prefer a dedicated resp_msg field or a flags bit? The compatibility argument above depends on the current completion test staying "non-null only", and you may reasonably want that invariant made explicit rather than relied upon.

Related: #109
Related: #118
Related: ClickHouse/ClickHouse#112405