Guard rollback-sensitive transaction flags

[Amaury Chamayou (achamayou)]

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

The problem

A transaction can request two side effects that outlive it:

• TxFlag::LEDGER_CHUNK_AT_NEXT_SIGNATURE - force a ledger chunk boundary
• TxFlag::SNAPSHOT_AT_NEXT_SIGNATURE - arm a snapshot

CommittableTx::commit() applied both immediately after allocating a version and applying its writes, before Store::commit() decides whether the transaction can be replicated at all. Neither application rechecked the transaction's view or rollback epoch, so if a concurrent election rolled the store back in that window:

• the forced chunk boundary was recorded for a version the ledger no longer contains, skewing every subsequent chunk;
• SNAPSHOT_AT_NEXT_SIGNATURE was re-armed on the store after Store::rollback() had deliberately cleared it, so the node takes a snapshot it was told not to take.

Separately, force_end_of_chunk was called with version read back off the transaction after the fact. That is the transaction's own version today, but it is trivially fragile: any caller reaching this path with a stale handle attaches the boundary to the wrong entry.

The fix

Both effects move behind a single new AbstractStore::apply_tx_flags, which takes version_lock once and applies them only if the transaction's view and rollback epoch still hold. On failure the transaction returns FAIL_NO_REPLICATE, which is what Store::commit() would have returned a moment later anyway - the transaction did not reach the ledger, so it must not leave anything behind.

The read-only (NoVersion) case is preserved explicitly: such a transaction has no version to attach a chunk to, but arming a snapshot is still legal and unchanged from before.

Why this is minimal

• One new store method, one call site.
• No new locks: it reuses version_lock, taken exactly where set_flag already took it. Lock order (commit_lock -> version_lock -> chunker_lock) is unchanged.
• Cost is one extra lock acquisition, only for transactions that actually set one of these two flags - i.e. signature-adjacent transactions, not the write path.

Test

Rollback-sensitive transaction flags are not restored in kv_test calls Store::apply_tx_flags directly - single-threaded, no harness - and asserts:

1. a stale rollback epoch/view is refused, with neither the flag set nor a chunk requested;
2. a current transaction applies both;
3. the chunk attaches to the transaction's own version, not the store's later version.

Mutation-verified: disabling the guard fails (1); using the store's version instead of the transaction's fails (3).

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]

🔵 Needs a closer look

The non-truncating rollback path can still advance a lagging chunker and permanently skip entry-size metadata.

Pull request overview

Guards KV transaction commits and side effects against concurrent rollbacks and view changes.

Changes:

• Validates commit terms before applying writes.
• Synchronizes transaction flags and chunk/snapshot metadata with rollback state.
• Adds regression tests and documents the API change.

Custom instructions used:

• .github/copilot-instructions.md
• .github/instructions/changelog.instructions.md
• .github/instructions/reviewing.instructions.md

File summaries

File	Description
src/kv/apply_changes.h	Supports rejected version resolution.
src/kv/committable_tx.h	Guards writes and transaction flags.
src/kv/kv_types.h	Extends the store interface.
src/kv/store.h	Coordinates rollback-sensitive state.
src/kv/test/kv_test.cpp	Adds rollback regression coverage.
src/node/rpc/frontend.h	Updates the commit call signature.
src/node/snapshotter.h	Updates the commit call signature.
CHANGELOG.md	Documents fixes and API changes.
python/pyproject.toml	Bumps the package version.

Review details

Suppressed comments (2)

src/kv/store.h:699

• This clamp still advances a lagging chunker. For example, if the store is at version 2 while current_tx_version is 1, a rollback to version 2 calls rolled_back_to(2), whose implementation assigns current_tx_version = 2; the size for entry 2 is then permanently skipped. The non-truncating path should not call rolled_back_to unless the requested version is below the chunker's own position (or rolled_back_to itself should be made monotonic backwards).

          }

src/kv/test/kv_test.cpp:3609

• This stale case changes both the term and rollback_count, so it still passes if the new term check in apply_tx_flags is accidentally removed. Add a separate non-truncating rollback case, where the term changes but the rollback count remains current, and verify that neither flag is applied.

  store.set_consensus(consensus);
  auto chunker = std::make_shared<InspectableChunker>();
  store.set_chunker(chunker);

  constexpr ccf::kv::Term initial_term = 2;
  store.initialise_term(initial_term);

• Files reviewed: 5/5 changed files
• Comments generated: 0
• Review effort level: Balanced

---

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