gc: GetGCState ignores request cancellation during storage reads
- Dominant language
- Go
- Stars
- 1.2k
- Forks
- 783
- Avg merge
- 5d 21h
- Merged PRs (30d)
- 36
Description
## Bug Report
### What did you do?
Issue a `GetGCState` request with a cancellable or deadline-bound context and make it enter a storage-backed path, such as a cache miss or a request with `include_global_gc_barriers = true`.
This can be reproduced in a test environment by blocking an underlying etcd read with a deterministic hook or test double, canceling the RPC context while the read is in progress, and then starting a GC-state write or a leader/follower transition that needs `GCStateManager.mu`.
### What did you expect to see?
Canceling the RPC should interrupt the associated storage work and release `GCStateManager.mu.RLock()` promptly. An abandoned request should not continue blocking GC-state writes or leader/follower transitions, and it should not populate the GC-state cache or return a partial result.
### What did you see instead?
The request context is dropped before the storage-backed GC-state read:
- [`GrpcServer.GetGCState`](https://github.com/tikv/pd/blob/3430f76dc5a48f93cb49a14aa25f010b79af5a76/server/gc_service.go#L690-L727) receives the RPC context but does not pass it to either manager read method.
- The existing [`GetGCState` slow path](https://github.com/tikv/pd/blob/3430f76dc5a48f93cb49a14aa25f010b79af5a76/pkg/gc/gc_state_manager.go#L796-L845) and the combined [`GetGCStateWithGlobalGCBarriers` path](https://github.com/tikv/pd/blob/3430f76dc5a48f93cb49a14aa25f010b79af5a76/pkg/gc/gc_state_manager.go#L910-L953) perform storage reads while holding `GCStateManager.mu.RLock()`, but neither accepts a context.
- [`GCStateProvider.RunInGCStateTransaction`](https://github.com/tikv/pd/blob/3430f76dc5a48f93cb49a14aa25f010b79af5a76/pkg/storage/endpoint/gc_states.go#L432-L493) has no context for its revision read, callback reads, or final raw transaction.
- The etcd [`Load` and `LoadRange` implementations](https://github.com/tikv/pd/blob/3430f76dc5a48f93cb49a14aa25f010b79af5a76/pkg/storage/kv/etcd_kv.go#L55-L91) use [`EtcdKVGet`](https://github.com/tikv/pd/blob/3430f76dc5a48f93cb49a14aa25f010b79af5a76/pkg/utils/etcdutil/etcdutil.go#L162-L180), which derives a new timeout from the etcd client context instead of the RPC context. The final raw transaction also uses an independent storage timeout.
For the combined path with local barriers excluded, the manager can execute the following storage operations while holding the read lock:
```text
revision read
-> transaction safe-point read
-> GC safe-point read
-> global-barrier range scan
-> revision-validation transaction
```
Including local barriers adds another range scan. Because these operations do not inherit the RPC context, a client may already have received `context deadline exceeded` while the PD server continues doing storage work and holding the read lock.
The abandoned read can delay:
- Transaction and GC safe-point advancement.
- Local and global GC barrier updates.
- GC-state cache invalidation during leader/follower transitions.
When `exclude_gc_barriers` is enabled, a storage-backed read may also populate the safe-point cache after the caller has canceled the request.
No unsafe GC advancement or data corruption has been identified. The observed risk is availability degradation and unnecessary etcd work.
### What version of PD are you using (`pd-server -V`)?
Current `master` after #11117 was merged:
```text
Git Commit Hash: 3430f76dc5a48f93cb49a14aa25f010b79af5a76
```
### Additional context
- [#10607](https://github.com/tikv/pd/issues/10607) previously identified the broader problem of performing etcd I/O while holding `GCStateManager.mu`, including the absence of context-aware `StorageEndpoint` APIs. It was closed by [#10677](https://github.com/tikv/pd/pull/10677), which added a leader-aware GC-state cache and changed storage-backed reads to use an `RLock`. Those changes reduce the frequency and contention of storage reads on the common path, but they do not propagate request cancellation into storage I/O.
- [#11117](https://github.com/tikv/pd/pull/11117) did not introduce the original cancellation gap. It added an opt-in combined read of one keyspace's GC state and all global GC barriers, which expands the exposure because that path always requires a storage-backed snapshot. The remaining gap was identified in [this review comment](https://github.com/tikv/pd/pull/11117#discussion_r3755753040) and deferred to a follow-up in [this reply](https://github.com/tikv/pd/pull/11117#discussion_r3755894476).
### Suggested scope
Propagate the request context through the storage-backed `GetGCState` paths:
- Pass the RPC context to `GCStateManager.GetGCState` and `GCStateManager.GetGCStateWithGlobalGCBarriers`.
- Propagate it through the shared slow-read helpers, the initial revision read, safe-point reads, barrier range scans, and final revision-validation transaction.
- Derive internal storage timeouts from the caller context so that the effective deadline is the earlier of the request deadline and the internal storage timeout.
- Check cancellation before updating the GC-state cache.
- Preserve the existing snapshot consistency, revision-conflict, and no-partial-result behavior.
Only adding a context parameter to `RunInGCStateTransaction` is insufficient if its underlying reads continue using context-free `EtcdKVGet`. Both the reads and final raw transaction need to use the supplied context.
The following work is related but out of scope:
- Limiting or paginating global GC barrier scans.
- Redesigning or removing `GCStateManager.mu`.
- Adding contexts to every legacy GC-state write API.
- Changing the GC-state cache consistency model.
### Acceptance criteria
- Cancellation during a safe-point read, barrier range scan, or final raw transaction interrupts the operation without waiting for an unrelated storage timeout.
- A canceled storage-backed read releases `GCStateManager.mu.RLock()` promptly.
- A representative GC-state writer and a leader/follower transition can proceed after the read context is canceled.
- A canceled read does not populate the GC-state cache or return a partial result.
- The normal and combined `GetGCState` paths are both covered by deterministic cancellation tests.
- Existing cache-hit, snapshot consistency, and revision-conflict behavior remains unchanged.
Contributor guide
Assessment
This issue has not been assessed yet.