Guard reserved signature side effects

[Amaury Chamayou (achamayou)]

Last of a stack of small fixes for a family of key-value store races found by an interleaving-exploration harness. Stacked on #8245.

The problem

CommittableTx::commit_reserved() decided whether a signature should end a ledger chunk, and recorded that decision on the chunker, in two steps:

// version_lock held by Store::commit
if (pimpl->store->should_create_ledger_chunk_unsafe(version))
{
  entry_flags |= EntryFlags::FORCE_LEDGER_CHUNK_AFTER;
  auto chunker = pimpl->store->get_chunker();
  if (chunker) { chunker->produced_chunk_at(version); }
}

That comment is wrong, and has been for some time. Store::commit() releases version_lock before it calls pending_tx->call(), which is what reaches this code - so the _unsafe read runs with no lock at all, and a rollback can land between the read and produced_chunk_at.

Two consequences:

• A stale chunk marker. The signature is discarded, but chunk_ends keeps an entry at its version. LedgerChunker::get_unchunked_size measures from the most recent chunk end, so every later chunk boundary is measured from an entry that is not in the ledger.
• A torn decision. The chunk flag on the entry and the chunker's record of it can disagree, because nothing holds them together.

Store::commit() already anticipates this shape of failure - it handles FAIL_NO_REPLICATE from pending_tx->call() with the comment "A pending tx may fail here if rollback invalidated a reserved signature tx after it was dequeued from pending_txs" - but commit_reserved() only produced it for a map-level conflict, never for its own side effects.

The fix

Store::prepare_reserved_tx takes version_lock once and does the whole thing atomically: validate the signature's view and rollback epoch, decide, and record. If the epoch no longer holds it returns nullopt and commit_reserved() returns FAIL_NO_REPLICATE - the path Store::commit() was already written to expect.

The stale comment is replaced with one that says what actually holds.

Why this is minimal

• One new store method, one call site; the decision logic itself is unchanged.
• No new locks. version_lock is not held here, so taking it is safe - and the existing order commit_lock -> version_lock -> chunker_lock is preserved, with LedgerChunker still a leaf.
• Cost is one lock acquisition per signature, which is not the write path.

Test

Reserved signature side effects are not applied after a rollback in kv_test calls Store::prepare_reserved_tx directly - single-threaded, no harness - covering all three cases:

1. a truncating rollback (new rollback epoch) is refused;
2. a non-truncating rollback that only moves the view is refused. This case matters: it discards nothing locally, so the snapshot flag survives and the pre-fix code would happily record a chunk end for a signature consensus is about to reject;
3. a signature still in its own epoch records its chunk end.

Mutation-verified: disabling the guard leaves exactly the stale chunk_ends marker described above.

Labelled run-long-test.

---

Review stack

These five PRs come from one investigation and are stacked; review and merge in order.

PR	Change
#8242	Reject stale-view writes before local commit
#8243	Order chunk metadata and snapshot scheduling with rollback
#8244	Stop a rollback moving chunk metadata forward
#8245	Guard rollback-sensitive transaction flags
#8246	Guard reserved signature side effects

All were found by an interleaving-exploration harness built over the real KV, consensus and history stack (draft #8238). The harness itself is deliberately not included here; these PRs carry only the fixes and the single-threaded regression tests that pin them.

[Copilot]

🟡 Changes recommended

The new code introduces an incorrect/deprecated mutex guard type usage that is likely to fail compilation and should be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a rollback race in reserved signature handling by making the “should end ledger chunk” decision and the chunker side-effect atomic with respect to rollback, preventing stale chunk-end markers from being recorded for signatures that never reach the ledger.

Changes:

• Add AbstractStore::prepare_reserved_tx() to validate term/rollback epoch under version_lock and (if still valid) record reserved-signature chunk side effects atomically.
• Update CommittableTx::commit_reserved() to use prepare_reserved_tx() and return FAIL_NO_REPLICATE when the reserved signature is no longer valid post-rollback.
• Add a single-threaded regression test in kv_test covering truncating rollback, view-only rollback, and the success case; document the fix in CHANGELOG.md.

Custom instructions used:

• None (no repository instruction files from .github/copilot-instructions.md or .github/instructions/ were loaded during this review).

File summaries

File	Description
src/kv/test/kv_test.cpp	Adds regression test ensuring reserved signature chunk side effects are skipped after rollback.
src/kv/store.h	Implements prepare_reserved_tx() under version_lock to guard chunk side effects against rollback/view changes.
src/kv/kv_types.h	Extends AbstractStore with prepare_reserved_tx() API.
src/kv/committable_tx.h	Uses prepare_reserved_tx() in commit_reserved() to avoid stale/torn chunk decisions across rollback.
CHANGELOG.md	Adds a “Fixed” entry describing the guarded reserved signature side effects.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.