Skip to content

transaction: refuse to resolve shared locks instead of mis-handling them - #556

Open
eduralph wants to merge 1 commit into
tikv:masterfrom
getwyrd:shared-locks
Open

transaction: refuse to resolve shared locks instead of mis-handling them#556
eduralph wants to merge 1 commit into
tikv:masterfrom
getwyrd:shared-locks

Conversation

@eduralph

@eduralph eduralph commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Stacked on #550 — please review the second commit only. Until #550 merges, the diff below also
contains its re-vendor.

Why: the re-vendored kvproto exposes shared locks (Op::SharedLock /
Op::SharedPessimisticLock). Their contract is unusual: a shared lock's real holders live only in
kvrpcpb.LockInfo.shared_lock_infos, and the proto comment is explicit that you must "DO NOT read
from the wrapper LockInfo" — whose own lock_version and primary_lock are left unset (its key
is not; see below).

What: this client doesn't implement shared-lock resolution, and every partial handling of one
is worse than none — resolving the wrapper checks transaction 0; filtering on the wrapper's fields
(use_async_commit, lock_type) silently drops the real members; and the pessimistic-lock special
cases in the resolver don't know SharedPessimisticLock. So resolution refuses them with an
explicit error: in resolve_locks, in LockResolver's crate-public entry point, and in
CleanupLocks::execute before any filter runs. Until real support lands, an explicit error is the
only answer that cannot roll back a live transaction or skip a dead one. Servers predating shared
locks never produce them, so this is a no-op there.

The API-v2 keyspace codecs are made shared-lock-aware separately, because they run on scan results
that never reach the resolver. They now take the shape of client-go's codecV2.decodeLockInfo
(internal/apicodec/codec_v2.go), which decodes a LockInfo's own Key / PrimaryLock /
Secondaries and then recurses into SharedLockInfos, with no wrapper special case. This client
now does the same: convert the lock's own key fields, then recurse into the members.

What the wrapper actually contains. An earlier revision skipped a wrapper's own fields entirely,
reading the proto's "DO NOT read from the wrapper LockInfo" as covering them. Checking the writer —
SharedLocks::into_lock_info in TiKV's components/txn_types/src/lock.rs, identical at v8.5.7
and on master — that's only half right, and the half it gets wrong matters:

info.set_shared_lock_infos(shared_locks.into());
info.set_key(raw_key);

A wrapper sets lock_type, shared_lock_infos and key — the key it locks — and leaves
primary_lock/lock_version at their defaults. Each member is built from that same raw key plus
its own primary and version. So the caveat scopes the per-transaction fields, not the locked key.
Skipping the wrapper wholesale would have handed scan_locks a physical key beside decoded member
keys; converting it wholesale would have hit the length assertion on the unset primary. Hence no
wrapper special case, just a per-field guard — both fields are exercised by tests.

I'd still welcome a correction if that reading of the writer is off. It can't be settled by test at
today's pin: CI runs TIKV_VERSION: v8.5.5, which predates shared locks, so no integration test in
this repo can produce one — the coverage below is unit-level by necessity, and the refusal path is
unreachable against that server. Worth noting for a separate change: v8.5.6 is the first release
carrying shared locks, so bumping the CI pin within the same release family would make these paths
reachable. Not proposing it here, given this PR is already a split-out.

The two directions guard differently, on purpose:

  • Truncating, empty bytes can only mean unset (an encoded key always carries its 4-byte
    prefix), so unset fields are skipped rather than run into pretruncate_bytes' length assertion.
  • Encoding, the input is a logical key and the empty logical key is valid in API v2, so
    nothing is skipped. scan_locksresolve_locks round-trips locks through truncate-then-encode,
    and skipping empties there would strand a lock on the empty key with no prefix, sending resolution
    after an empty physical key. Wrappers need no encode-side guard because resolution refuses them
    first.

The panic hazard predates the re-vendor: a wrapper arrives the same way whichever proto vintage
parses it.

Full shared-lock support is deliberately follow-up work.

Verification: 4 new tests — the resolver refusal (a plain lock passes; a wrapper carrying members
and a lock marked shared only by its op are both refused); the codec converting a wrapper's own key
alongside its members; a wrapper's unset fields surviving truncation (the other reading, so both are
pinned); and a lock on the empty logical key round-tripping through truncate-then-encode. 71 lib
tests; make check green; txn/raw/failpoint integration suites green against a local api-v2 cluster;
and behaviour compared against client-go by driving both clients through identical scenarios — no
observable change.

Summary by CodeRabbit

  • Bug Fixes
    • Improved transaction lock cleanup when lock fields are missing, empty, or malformed.
    • Prevented shared-lock records from being mistakenly treated as ordinary locks during cleanup.
    • Preserved empty logical keys through encoding and decoding.
    • Added validation so unsupported shared-lock representations return an error instead of being silently altered.
    • Improved handling of nested lock information and lock cleanup reliability.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has signed the dco. labels Jul 25, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andremouche for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a477f76-4d25-4f31-8fc0-885631bf386c

📥 Commits

Reviewing files that changed from the base of the PR and between 6e07351 and 4a28830.

📒 Files selected for processing (4)
  • src/request/keyspace.rs
  • src/request/plan.rs
  • src/transaction/lock.rs
  • src/transaction/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/transaction/mod.rs
  • src/transaction/lock.rs
  • src/request/plan.rs
  • src/request/keyspace.rs

📝 Walkthrough

Walkthrough

The changes update keyspace conversion for unset and nested shared-lock fields. Transaction lock resolution and cleanup now reject shared-lock representations before processing. Tests cover conversion, malformed entries, accepted locks, and rejected variants.

Changes

Shared-lock safety updates

Layer / File(s) Summary
Keyspace conversion for shared-lock fields
src/request/keyspace.rs
Lock truncation skips unset fields and recursively processes nested shared-lock members. Encoding preserves empty logical keys and nested member keys. Tests cover empty keys, unset fields, and malformed secondary entries.
Shared-lock rejection before processing
src/transaction/lock.rs, src/transaction/mod.rs, src/request/plan.rs
reject_shared_locks detects shared-lock variants. Lock resolution and cleanup call the helper before processing. CleanupLocks::execute validates locks before filtering. Tests cover accepted ordinary locks and rejected shared-lock variants.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: lgtm, approved

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: refusing to resolve shared locks instead of mishandling them.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added contribution This PR is from a community contributor. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 25, 2026
@eduralph
eduralph marked this pull request as ready for review July 25, 2026 22:12
@ti-chi-bot ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 25, 2026

@pingyu pingyu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM.

Comment thread src/request/keyspace.rs
The re-vendored kvproto exposes shared locks (Op::SharedLock /
Op::SharedPessimisticLock). Their contract is unusual: a shared lock's real
holders live ONLY in kvrpcpb.LockInfo.shared_lock_infos — the proto comment is
explicit that you must "DO NOT read from the wrapper LockInfo", whose own key
and lock_version are unset.

This client does not implement shared-lock resolution, and every partial
handling of one is worse than none: resolving the wrapper checks transaction 0;
filtering on the wrapper's fields (use_async_commit, lock_type) silently drops
the real members; and the pessimistic-lock special cases in the resolver do not
know SharedPessimisticLock. So resolution REFUSES them with an explicit error —
in resolve_locks, in LockResolver's crate-public entry point, and in
CleanupLocks::execute before any filter runs. Until real support lands, an
explicit error is the only answer that cannot roll back a live transaction or
skip a dead one. Servers that predate shared locks never produce them, so this
is a no-op there.

The API-v2 keyspace codecs are made shared-lock-aware separately, because they
run on scan results that never reach the resolver. They now take the shape of
client-go's codecV2.decodeLockInfo (internal/apicodec/codec_v2.go), which
decodes a LockInfo's own Key/PrimaryLock/Secondaries and then recurses into
SharedLockInfos — it has no wrapper special case. This client now does the
same: convert the lock's OWN key fields, then recurse into the members.

An earlier revision skipped a wrapper's own fields entirely, reading the proto's
"DO NOT read from the wrapper LockInfo" as covering them. Checking the writer
(TiKV's SharedLocks::into_lock_info in components/txn_types/src/lock.rs,
identical at v8.5.7 and on master) shows
that is only half right, and the half it gets wrong matters:

  info.set_shared_lock_infos(shared_locks.into());
  info.set_key(raw_key);

A wrapper sets lock_type, shared_lock_infos and key — the key it locks — and
leaves primary_lock and lock_version at their defaults; each member is built
from that SAME raw key plus its own primary and version. So the caveat scopes
the per-transaction fields, not the locked key. Skipping the wrapper wholesale
would have handed scan_locks a physical key beside decoded member keys, while
converting it wholesale would have hit the length assertion on the unset
primary. Both fields are exercised by tests.

The two directions guard differently, on purpose. Truncating,
empty bytes can only mean "unset" — an encoded key always carries its 4-byte
prefix — so unset fields are skipped rather than run into pretruncate_bytes'
length assertion. Encoding, the input is a LOGICAL key and the empty logical key
is valid in API v2, so nothing is skipped: scan_locks -> resolve_locks
round-trips locks through truncate-then-encode, and skipping empties there would
strand a lock on the empty key with no prefix and send resolution after an empty
physical key. Wrappers need no encode-side guard because resolution refuses them
first. That panic hazard predates the re-vendor: a wrapper arrives the same way
whichever proto vintage parses it.

Full shared-lock support is deliberately follow-up work.

Split out of the kvproto re-vendor at pingyu's suggestion (tikv#550).

Tests: 4 new — the resolver refusal (a plain lock passes; a wrapper carrying
members and a lock marked shared only by its op are both refused); the codec
converting a wrapper's own key alongside its members; a wrapper's unset fields
surviving truncation (the other reading, so both are pinned); and a lock on the
empty logical key round-tripping through truncate-then-encode. 71 lib tests
green.

The coverage is unit-level by necessity: CI pins TIKV_VERSION v8.5.5, which
predates shared locks, so no integration test in this repo can produce one, and
the refusal path is unreachable against that server. v8.5.6 is the first release
carrying shared locks, so a CI pin bump within the same release family would
make these paths reachable — left to a separate change.

Signed-off-by: Eduard R. <eduard@ralphovi.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution This PR is from a community contributor. dco-signoff: yes Indicates the PR's author has signed the dco. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants