request: treat an undetermined apply outcome as terminal at commit points#555
Open
eduralph wants to merge 3 commits into
Open
request: treat an undetermined apply outcome as terminal at commit points#555eduralph wants to merge 3 commits into
eduralph wants to merge 3 commits into
Conversation
…revision The vendored protos were an unrecorded late-2023/early-2024 vintage (errorpb and pdpb untouched since tikv#324, one hand-patched addition from tikv#519). Nothing recorded which kvproto revision they came from, and the staleness now blocks real features. Re-vendor everything at pingcap/kvproto b41e86365ce0 — the revision client-go currently pins — and add proto/VERSION so the vintage is never again guesswork. Regenerated with the existing tonic-build pipeline (21 files). One mechanical source fix: pdpb.RequestHeader gained caller_component/caller_id, so the TSO stream's header construction now defaults the new fields. This commit is the re-vendor and nothing else. Newly available surface includes KvFlush / KvBufferBatchGet (pipelined DML), HealthFeedback, and pdpb.BatchScanRegions. Two new protocol semantics become VISIBLE with the refresh — errorpb.UndeterminedResult (an unknown raft apply outcome) and shared locks (SharedLock / SharedPessimisticLock, whose real holders live in shared_lock_infos). Handling them is deliberately left to follow-up PRs so this one stays a pure re-vendor. Neither is a regression introduced here: - UndeterminedResult falls through to the generic region-error arm, which backs off and retries — the same default the follow-up keeps for every plan that is not a commit point. - A shared-lock wrapper carries no key bytes on the wire regardless of which proto vintage parses it, so the keyspace truncation hazard predates this commit; the refresh only makes the wrapper identifiable. Tests: make check green; 67 lib tests; txn/raw/failpoint integration suites green against a local api-v2 cluster. Signed-off-by: Eduard R. <eduard@ralphovi.net>
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis refreshes vendored kvproto definitions and code-generation options, adds multiple protocol messages and RPCs, and changes multi-region retry handling so selected CAS and transaction operations surface undetermined outcomes without replaying them. ChangesProtocol and vendoring refresh
Undetermined retry handling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
contribution
eduralph
force-pushed
the
undetermined-result
branch
from
July 25, 2026 13:49
81249dd to
eddb093
Compare
…ints
The re-vendored kvproto exposes errorpb.UndeterminedResult: the server telling
us the raft apply outcome is UNKNOWN. Today it falls through to the generic
region-error arm and is retried like any transient failure. For most requests
that is right, but not for a commit point, where "failed" must never be claimed
for a transaction that may already be durable.
Follow client-go's shape: its transport never retries UndeterminedResult
("should not retry ... processed by the caller", region_request.go) and each
ACTION decides. So the plan layer keeps RETRY as the default — replaying an
idempotent request resolves the uncertainty, and on backoff exhaustion the
error escapes UNCHANGED so callers can still classify it — while plans for
which a replay is unsafe opt in to terminal-on-first-sight via
retry_multi_region_terminal_on_undetermined:
- raw CAS: a replay would compare against its OWN effect and report
succeed = false for a write that happened.
- the primary commit: client-go returns ErrResultUndetermined here (commit.go)
rather than retrying.
- async-commit / 1PC prewrites: the prewrite IS the commit point. Stricter
than client-go, in the safe direction.
Shard aggregation is fixed alongside: collect::<Result<_>>() returns the first
error BY SHARD INDEX, so a lower-index shard's determinate error could mask a
higher-index shard's UndeterminedResult and tell the caller the request
definitely failed. An undetermined error from any shard now wins.
Commit paths classify the surfaced error as UndeterminedError. Two places are
deliberately conservative and say so in comments: a 1PC prewrite that sharding
downgraded to 2PC, and an undetermined error arriving from the commit plan's
resolve_lock layer rather than the commit dispatch. Both only over-report
uncertainty, which makes the caller verify instead of replay.
Split out of the kvproto re-vendor at pingyu's suggestion (tikv#550).
Tests: 4 new — terminal for opted-in plans under a generous backoff (proving
terminal, not retry-exhausted), preserved unchanged on retry exhaustion,
recognizer positive/negative, and the shard-masking hazard. 71 lib tests green.
Signed-off-by: Eduard R. <eduard@ralphovi.net>
eduralph
force-pushed
the
undetermined-result
branch
from
July 25, 2026 22:03
eddb093 to
4f0c54c
Compare
eduralph
marked this pull request as ready for review
July 25, 2026 22:12
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proto/encryptionpb.proto (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuf compile errors here are an include-path artifact, not a file defect.
gogoproto/gogo.protoandrustproto.protolive underproto/include/; Buf resolves imports relative to its module root, so every vendored file reports the same unresolvable-option/import errors. Consider adding abuf.yaml/buf.work.yamlthat registersprotoandproto/includeas roots (or excluding vendored protos from Buf) so CI signal isn't drowned in false positives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proto/encryptionpb.proto` around lines 12 - 18, Update the Buf configuration rather than proto/encryptionpb.proto: register both proto and proto/include as module roots so imports of gogoproto/gogo.proto and rustproto.proto resolve, or exclude the vendored protos from Buf validation. Ensure CI no longer reports false-positive compile errors for these vendored files.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@proto/encryptionpb.proto`:
- Around line 12-18: Update the Buf configuration rather than
proto/encryptionpb.proto: register both proto and proto/include as module roots
so imports of gogoproto/gogo.proto and rustproto.proto resolve, or exclude the
vendored protos from Buf validation. Ensure CI no longer reports false-positive
compile errors for these vendored files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff4ddb26-826e-403b-b41f-609a275ef6b7
⛔ Files ignored due to path filters (21)
src/generated/backup.rsis excluded by!**/generated/**src/generated/cdcpb.rsis excluded by!**/generated/**src/generated/coprocessor.rsis excluded by!**/generated/**src/generated/deadlock.rsis excluded by!**/generated/**src/generated/encryptionpb.rsis excluded by!**/generated/**src/generated/errorpb.rsis excluded by!**/generated/**src/generated/import_sstpb.rsis excluded by!**/generated/**src/generated/keyspacepb.rsis excluded by!**/generated/**src/generated/kvrpcpb.rsis excluded by!**/generated/**src/generated/logbackup.rsis excluded by!**/generated/**src/generated/meta_storagepb.rsis excluded by!**/generated/**src/generated/metapb.rsis excluded by!**/generated/**src/generated/mpp.rsis excluded by!**/generated/**src/generated/pdpb.rsis excluded by!**/generated/**src/generated/raft_cmdpb.rsis excluded by!**/generated/**src/generated/raft_serverpb.rsis excluded by!**/generated/**src/generated/resource_manager.rsis excluded by!**/generated/**src/generated/resource_usage_agent.rsis excluded by!**/generated/**src/generated/schedulingpb.rsis excluded by!**/generated/**src/generated/tikvpb.rsis excluded by!**/generated/**src/generated/tsopb.rsis excluded by!**/generated/**
📒 Files selected for processing (42)
proto/VERSIONproto/autoid.protoproto/brpb.protoproto/cdcpb.protoproto/configpb.protoproto/coprocessor.protoproto/deadlock.protoproto/debugpb.protoproto/diagnosticspb.protoproto/disaggregated.protoproto/disk_usage.protoproto/encryptionpb.protoproto/enginepb.protoproto/errorpb.protoproto/gcpb.protoproto/import_kvpb.protoproto/import_sstpb.protoproto/include/eraftpb.protoproto/include/gogoproto/gogo.protoproto/include/rustproto.protoproto/keyspacepb.protoproto/kvrpcpb.protoproto/logbackuppb.protoproto/meta_storagepb.protoproto/metapb.protoproto/mpp.protoproto/pdpb.protoproto/raft_cmdpb.protoproto/raft_serverpb.protoproto/recoverdatapb.protoproto/replication_modepb.protoproto/resource_manager.protoproto/resource_usage_agent.protoproto/schedulingpb.protoproto/tikvpb.protoproto/tracepb.protoproto/tsopb.protosrc/pd/timestamp.rssrc/raw/client.rssrc/request/plan.rssrc/request/plan_builder.rssrc/transaction/transaction.rs
ti-chi-bot
Bot
added
size/XL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why: the re-vendored kvproto exposes
errorpb.UndeterminedResult— the server telling us theraft apply outcome is UNKNOWN. Today it falls through to the generic region-error arm and is retried
like any transient failure. For most requests that's right; for a commit point it isn't. "Failed"
must never be claimed for a transaction that may already be durable.
What: follow client-go's shape. Its transport never retries
UndeterminedResult("should notretry … processed by the caller",
region_request.go) and each action decides. So the plan layerkeeps retry as the default — replaying an idempotent request resolves the uncertainty, and on
backoff exhaustion the error escapes unchanged so callers can still classify it — while plans for
which a replay is unsafe opt in to terminal-on-first-sight:
succeed = falsefor awrite that happened.
ErrResultUndeterminedhere (commit.go) rather thanretrying.
the safe direction.
Shard aggregation is fixed alongside:
collect::<Result<_>>()returns the first error by shardindex, so a lower-index shard's determinate error could mask a higher-index shard's
UndeterminedResultand tell the caller the request definitely failed. An undetermined error fromany shard now wins.
Commit paths classify the surfaced error as
UndeterminedError. Two places are deliberatelyconservative and say so in comments: a 1PC prewrite that sharding downgraded to 2PC, and an
undetermined error arriving from the commit plan's
resolve_locklayer rather than the commitdispatch. Both only over-report uncertainty, which makes the caller verify instead of replay.
Verification: 4 new tests — terminal for opted-in plans under a generous backoff (proving
terminal, not merely retry-exhausted), preserved unchanged on retry exhaustion, recognizer
positive/negative, and the shard-masking hazard. 71 lib tests;
make checkgreen; txn/raw/failpointintegration suites green against a local api-v2 cluster.
Summary by CodeRabbit
New Features
Bug Fixes