Two independent bugs found during a code review of already-merged code (the resilient-startup and zstd-log-compression work). Both are startup/upgrade data-safety issues, verified against current master.
1. Snapshot-fallback gap-abort still crashes the node it's meant to save
src/Service/NuRaftStateMachine.cpp (~line 104), in the snapshot-loading fallback loop:
ulong log_start = log_store_->start_index();
if (last_committed_idx + 1 < log_start)
{
LOG_WARNING(log, "Snapshot ... aborting fallback.", ...);
store.reset();
last_committed_idx = 0; // <-- resets to 0
break;
}
After the break, the log-replay decision block below runs:
if (previous_last_commit_id == 0) { /* skip */ }
else if (previous_last_commit_id <= last_committed_idx) { /* skip */ }
else
replayLogs(log_store_, last_committed_idx + 1, previous_last_commit_id); // replayLogs(_, 1, previous)
With last_committed_idx == 0 and previous_last_commit_id > 0, the else fires and calls replayLogs(log_store_, 1, previous_last_commit_id) against a log store whose start_index() > 1. replayLogs detects the gap and throws GAP_BETWEEN_SNAPSHOT_AND_LOG, aborting startup — on exactly the node the fallback was supposed to rescue, after logging a misleading "proceeding with empty state".
Repro shape: a node whose newest loadable snapshot has last_log_idx = S while its log store has been compacted to start_index > S + 1 (e.g. crash during NuRaft's install-snapshot window, which compacts the log before applying the snapshot; or a backup snapshot paired with compacted logs).
Suggested fix: set last_committed_idx = log_start - 1 (so replay starts at log_start) instead of 0, or skip the replay block when the fallback gap-abort fired.
2. V1→V2 log upgrade corrupts every entry appended post-upgrade to an existing open segment
src/Service/NuRaftLogSegment.cpp. appendEntry (~line 422) unconditionally prepends the 1-byte codec tag to the on-disk body:
on_disk_buf = buffer::alloc(1 + raw_body_size);
*on_disk_buf->data_begin() = static_cast<uint8_t>(LogEntryCodec::RAW);
memcpy(on_disk_buf->data_begin() + 1, raw_body, raw_body_size);
...
header.data_length = on_disk_buf->size(); // includes the tag byte
…but loadEntry (~line 516) only strips that tag when version >= V2:
if (version >= LogVersion::V2)
{
auto codec = static_cast<LogEntryCodec>(*buf->data_begin());
// strip first byte, decode payload
}
// else: body = whole buffer, tag byte NOT stripped
An existing V1 open segment keeps version = V1 when reloaded (readHeader, ~line 325). After a binary upgrade, entries appended to that still-open V1 segment get the tag byte written but, on the next restart, are read back with version = V1 → the tag byte is not stripped → LogEntryBody::deserialize sees 0x00 (the RAW tag) as the first byte of the log type → every post-upgrade entry on that segment deserializes as garbage.
Suggested fix: either bump the open segment's version to V2 (and rewrite its header) when the first post-upgrade entry is appended, or gate the tag-prepend in appendEntry on version >= V2 so V1 segments stay tag-free until they roll.
Both are latent until the specific upgrade/crash path is hit, but both destroy or refuse committed data when they are, so filing for visibility. Not touching them in #400 (that PR's diff doesn't include these files); they belong with the resilient-startup / zstd-log owners.
Two independent bugs found during a code review of already-merged code (the resilient-startup and zstd-log-compression work). Both are startup/upgrade data-safety issues, verified against current
master.1. Snapshot-fallback gap-abort still crashes the node it's meant to save
src/Service/NuRaftStateMachine.cpp(~line 104), in the snapshot-loading fallback loop:After the
break, the log-replay decision block below runs:With
last_committed_idx == 0andprevious_last_commit_id > 0, theelsefires and callsreplayLogs(log_store_, 1, previous_last_commit_id)against a log store whosestart_index() > 1.replayLogsdetects the gap and throwsGAP_BETWEEN_SNAPSHOT_AND_LOG, aborting startup — on exactly the node the fallback was supposed to rescue, after logging a misleading "proceeding with empty state".Repro shape: a node whose newest loadable snapshot has
last_log_idx = Swhile its log store has been compacted tostart_index > S + 1(e.g. crash during NuRaft's install-snapshot window, which compacts the log before applying the snapshot; or a backup snapshot paired with compacted logs).Suggested fix: set
last_committed_idx = log_start - 1(so replay starts atlog_start) instead of0, or skip the replay block when the fallback gap-abort fired.2. V1→V2 log upgrade corrupts every entry appended post-upgrade to an existing open segment
src/Service/NuRaftLogSegment.cpp.appendEntry(~line 422) unconditionally prepends the 1-byte codec tag to the on-disk body:…but
loadEntry(~line 516) only strips that tag whenversion >= V2:An existing V1 open segment keeps
version = V1when reloaded (readHeader, ~line 325). After a binary upgrade, entries appended to that still-open V1 segment get the tag byte written but, on the next restart, are read back withversion = V1→ the tag byte is not stripped →LogEntryBody::deserializesees0x00(the RAW tag) as the first byte of the log type → every post-upgrade entry on that segment deserializes as garbage.Suggested fix: either bump the open segment's version to V2 (and rewrite its header) when the first post-upgrade entry is appended, or gate the tag-prepend in
appendEntryonversion >= V2so V1 segments stay tag-free until they roll.Both are latent until the specific upgrade/crash path is hit, but both destroy or refuse committed data when they are, so filing for visibility. Not touching them in #400 (that PR's diff doesn't include these files); they belong with the resilient-startup / zstd-log owners.