Reject stale-view writes before local commit

[Amaury Chamayou (achamayou)]

Why this is necessary

CommittableTx::commit() documents that "transactions that fail are rolled back, no matter the reason". That was not true for a view change.

A transaction reads state (fixing its commit view), then applies its writes and allocates a version, and only later reaches Store::commit() where its view is checked against the store's. If an election lands in that window, the transaction is refused with FAIL_NO_REPLICATE - but its write is already applied locally, with no corresponding entry ever reaching consensus. Worse, because last_replicated no longer matches the store's version, ordinary transactions committed afterwards can keep succeeding locally without replicating, until a further election restores agreement.

This was found by driving a real Store, MerkleTxHistory and aft::Aft together under pinned election interleavings. The harness that found it is not proposed for merge; the regression here reproduces the same failure with the existing stubs.

What changes

The transaction's captured view is validated atomically with the allocation of its version, under the same version_lock a rollback takes:

• if the view moved first, no version is allocated and no map is touched - the transaction is refused before it can leave anything behind;
• if allocation wins the race, the rollback necessarily observes the new version and truncates the writes.

FAIL_NO_REPLICATE therefore no longer implies a locally applied write.

Why this is minimal

Version allocation is the only point that is already atomic with rollback, so it is the only place the check can be made without introducing new lock ordering between the KV and consensus. The alternative - repairing the store after the fact - would have to roll back writes belonging to unrelated concurrent transactions.

Two consequential simplifications come with it:

• next_version() returns the rollback epoch observed at allocation, which later fixes in this stack build on.
• The caller-supplied version resolver parameter of commit() is removed. It had no callers in tree, and any future user of it would have silently bypassed this check.

Testing

kv_test gains "Stale-view writes are rejected before local application", which covers both an existing map and a dynamically created one, and asserts that replication continues normally afterwards without a healing election. It is single-threaded and deterministic.

Verified that the test fails without the fix (6 assertions, including the stale value and dynamic map surviving), and passes with it. Also exercised under ThreadSanitizer.

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]

Pull request overview

This PR fixes a KV-store consistency bug where transactions could apply writes locally and then fail to replicate after a view/term change, leaving unreplicated state behind. It moves the view validation to be atomic with version allocation (under version_lock), ensuring stale-term transactions are rejected before they can modify any maps.

Changes:

• Make version allocation (Store::next_version) term-aware and able to refuse stale-view commits before any map commit occurs.
• Remove the unused caller-supplied version-resolver parameter from CommittableTx::commit() and update call sites accordingly.
• Add a regression test to ensure stale-view transactions do not leave local state behind (including for dynamically created maps), and bump version/changelog.

Custom instructions used:

• .github/skills/testing/SKILL.md
• .github/instructions/changelog.instructions.md

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/node/snapshotter.h	Update CommittableTx::commit() call to new signature (removed version resolver arg).
src/node/rpc/frontend.h	Update CommittableTx::commit() call to new signature.
src/kv/test/kv_test.cpp	Add regression test covering stale-view rejection before local application (existing + dynamic maps).
src/kv/store.h	Enforce term check during version allocation; simplify view-check in Store::commit().
src/kv/kv_types.h	Update store interface to return optional version resolution including rollback epoch/count.
src/kv/committable_tx.h	Plumb new version resolution API; map stale-term refusal to FAIL_NO_REPLICATE.
src/kv/apply_changes.h	Defer map commits until version resolution succeeds; allow resolver to fail (std::optional).
python/pyproject.toml	Bump project version to 7.0.14 to match changelog.
CHANGELOG.md	Add 7.0.14 entries documenting the fix and API change (with PR reference).

Suppressed comments (1)

src/kv/test/kv_test.cpp:3029

• ReadableMapHandle::get() returns std::optional<std::string>, so comparing it directly to a string literal is not a valid comparison on older language standards and will fail to compile on most toolchains. Compare against an optional (or unwrap the optional explicitly).

    auto tx = store.create_read_only_tx();
    CHECK(tx.ro(map)->get(key) == "fresh");
  }

---

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

[Amaury Chamayou (achamayou)]

CI note: Long Shuffled/LTS/Snmalloc failed in the shuffled full_test_suite, not in LTS. governance_history.test_ledger_is_readable timed out waiting 5 seconds for backup node 10 to expose its local ledger chunk. This is the known backup-synchronisation race tracked by #8248, not a failure in this PR's KV change. The other jobs, including ASAN, TSAN, and both long e2e variants, passed.